[katello-devel] Apipie and validation.

Wed Dec 5 12:39:42 UTC 2012

On 05/12/12 12:20 PM, Ohad Levy wrote:
>
> ----- Original Message -----
> | On 05/12/12 09:07 AM, Petr Chalupa wrote:
> | > I agree that domain validation/consistency checks belongs to the
> | > models. Nevertheless definition of the API is other thing entirely.
> | > It
> | > should be defined and validated on its own with re-usage of
> | > information provided by models (domain) to avoid duplications. It
> | > should not be delegated to model to deal with it by adding other
> | > responsibility.
> |
> | This statement is only valid if we are talking about cross-concerns
> | (think authentication, security-related stuff, etc). Everything else
> | will result in duplicated, non-consistent validations. Take a look at
> | current state of validations in foreman api.
> |
> What do you mean by that?

Which part? I insist that controllers should not carry any business 
logic in them, as a corollary to this, no validations should reside in 
controllers, unless those are used across all controllers, and are 
outside of business-logic constraints (think aspect-like behaviour).

By current state I meant that there are currently inconsistencies (data 
coming through api are flagged invalid, even though it's valid according 
to validators on the model) between api and model validations. 
Config_template has a few, for example (and what started this thread).

-d
>
> | I don't want to be jumping through hoops, and fixing/adding
> | code/meta-data in multiple places, so just the documentation is
> | up-to-date. It's a process/organizational issue, and tools do not fix
> | those.
> |
> | And, once again, validations belong in the model. Let's get rid of
> | validations in apipie, fix issues we have with domain logic leaking
> | into
> | controllers.
> |
> | >
> | > So I strongly suggest to fix the duplication issue [1] and keep
> | > things
> | > separate.
> | > * Apipie for API validations.
> | > * Model validations for domain logic.
> | >
> | > ## Fix Apipie
> | >
> | > Enable Apipie to read model definitions and validations to set
> | > param
> | > validations automatically for parts of request representing a
> | > domain
> | > model, see [1].
> |
> | Agree with defining docs based on model (and I would suggest routes).
> | Disagree with everything else. No point in intercepting validations,
> | the
> | most you could do in the controller is map exceptions thrown by the
> | model to http errors/statuses.
> |
> | >
> | >
> | > + api documentation generated from api specification
> | > + api specification is kept up to date (has to be because
> | > validation
> | > is turned on)
> | > + api validation/specification is separated from domain
> | > validations/logic
> | >
> | > ## Remove validations from Apipie
> | >
> | > + api documentation generated from api specification
> | > - api specification is not kept up to date (has to be ensured in an
> | > extra task)
> | > - no api validation, it's partially handled by models, errors can
> | > be
> | > ugly, models have another responsibility
> |
> | Again, tools are not the way to enforce process deficiencies. The
> | last
> | statement is FUD.
> |
> | >
> | >
> | > [1] - https://github.com/Pajk/apipie-rails/issues/67
> | >
> | > ## Other notes
> | >
> | > > The second issue is that apipie validations are stricter than AR
> | > ones: array validation assumes that there's an array in place,
> | > while
> | > AR would accept a nil. These two combined lead to even more false
> | > positives during apipie validation.
> | >
> | > I think be more consistent and strict is a good thing, it prevents
> | > confusion. Empty Array is an empty Array not a nil.
> |
> | It's not good, because depending on how you use foreman, you'll get
> | different results. What's considered valid/correct by foreman proper
> | should be valid by any other means, period.
> |
> | -d
> |
> | >
> | > Petr
> | >
> | > On 04.12.12 22:47, Ivan Necas wrote:
> | >>
> | >>
> | >> ----- Original Message -----
> | >>> On Tue 04 Dec 2012 05:10:28 AM MST, Dmitri Dolguikh wrote:
> | >>>> On 03/12/12 09:36 AM, Lukas Zapletal wrote:
> | >>>>>> Step #1 - let's get rid of controller validations.
> | >>>>> Again, you can only do this for CRUD actions in the
> | >>>>> controllers.
> | >>>>> For all
> | >>>>> other methods we need validations. And we have those, not all
> | >>>>> our
> | >>>>> controllers are plain REST-driven. We do have procedure-like
> | >>>>> actions.
> | >>>>> Maybe more then we should have, but this is a fact.
> | >>>>>
> | >>>>> You say all the validations should be done in models, thus you
> | >>>>> essentially say let's put all our code logic there. Because it
> | >>>>> does not
> | >>>>> make sense to do validations in models while doing anything
> | >>>>> else
> | >>>>> (that
> | >>>>> also requires some level of validations) in controllers or a
> | >>>>> logic
> | >>>>> layer. A fact: you need to do validations also outside of
> | >>>>> models.
> | >>>>
> | >>>> Domain logic belongs in the model layer. We can discuss how to
> | >>>> better
> | >>>> factor model layer code, but not where to put it.
> | >>>>
> | >>>> -d
> | >>>>
> | >>>> A lot of rationalizations to keep validations in apipie have
> | >>>> been
> | >>>> put
> | >>>> forward, *all* of them attempting to deal with various smells,
> | >>>> code
> | >>>> or
> | >>>> process ones. Let's fix the root issues.
> | >>>>
> | >>>> Again, Let's get rid of validations in apipie. Refactor the
> | >>>> controllers when needed. If apipie is not useful without those
> | >>>> validations, let's suspend/stop use of it.
> | >>
> | >> Am I reading it correctly you suggesting getting rid of API
> | >> documentation?
> | >>
> | >> -- Ivan
> | >>
> | >>>>
> | >>>> -d
> | >>>>
> | >>> +1
> | >>>>
> | >>>>>
> | >>>>> It is wrong to put all your code logic into models. And
> | >>>>> unfortunately we
> | >>>>> have most of it there. Rails follows MVC pattern and what we
> | >>>>> have
> | >>>>> is
> | >>>>> those
> | >>>>> messy Models-for-everything approach. If you mix this with our
> | >>>>> orchestration, it is deadly combination. Difficult testability,
> | >>>>> huge
> | >>>>> objects with no responsibility split, unable to mock things and
> | >>>>> so
> | >>>>> on.
> | >>>>> Code logic in models works well for small projects and it is
> | >>>>> de-facto
> | >>>>> standard in Rails world. People just blindly follow this wrong
> | >>>>> pattern.
> | >>>>>
> | >>>>> So I don't agree with this simplification you offer. We will
> | >>>>> likely
> | >>>>> start to refactor our orchestration creating new layer of code
> | >>>>> logic,
> | >>>>> which is the right approach to do things. And we should move
> | >>>>> complex
> | >>>>> methods like activation key subscription from models there.
> | >>>>> Then
> | >>>>> we will
> | >>>>> need validations on the controller side (or on the newly
> | >>>>> created
> | >>>>> logic
> | >>>>> layer or orchestration layer - depends on how are we gonna name
> | >>>>> it).
> | >>>>>
> | >>>>> So yes, it has something to do with REST. My counter proposal
> | >>>>> is:
> | >>>>>
> | >>>>> #2 Let's get rid of validations where they duplicate model
> | >>>>> validators,
> | >>>>> but let's keep them for (usually non-CRUD) methods which need
> | >>>>> then. We
> | >>>>> will likely be extending those soon.
> | >>>>>
> | >>>>
> | >>>> _______________________________________________
> | >>>> katello-devel mailing list
> | >>>> katello-devel at redhat.com
> | >>>> https://www.redhat.com/mailman/listinfo/katello-devel
> | >>>
> | >>>
> | >>>
> | >>> --
> | >>> Jason E. Rist
> | >>> Senior Software Engineer
> | >>> Systems Management and Cloud Enablement
> | >>> Red Hat, Inc.
> | >>> +1.919.754.4048
> | >>> Freenode: jrist
> | >>> github/identi.ca: knowncitizen
> | >>>
> | >>> _______________________________________________
> | >>> katello-devel mailing list
> | >>> katello-devel at redhat.com
> | >>> https://www.redhat.com/mailman/listinfo/katello-devel
> | >>>
> | >>
> | >> _______________________________________________
> | >> katello-devel mailing list
> | >> katello-devel at redhat.com
> | >> https://www.redhat.com/mailman/listinfo/katello-devel
> | >>
> | >
> | > _______________________________________________
> | > katello-devel mailing list
> | > katello-devel at redhat.com
> | > https://www.redhat.com/mailman/listinfo/katello-devel
> |
> | _______________________________________________
> | katello-devel mailing list
> | katello-devel at redhat.com
> | https://www.redhat.com/mailman/listinfo/katello-devel
> |