[katello-devel] Apipie and validation.

Petr Chalupa pchalupa at redhat.com
Fri Dec 7 08:29:47 UTC 2012



On 05.12.12 13:39, Dmitri Dolguikh wrote:
> 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).

Yes I agree that controllers should not carry any business logic, but 
they can define an API and they can validate if an user of the API is 
using it right, meaning validate the request before any other action is 
taken in domain. The issue is that the API validations are very similar 
to model validations in simple crud actions and we aren't using the 
model validation definition to construct API validations dynamically, we 
have to write them and that is the duplicity. This can be fixed by 
retrieving the already present information from models and construct API 
validations dynamically.

I would focus fixing [1] soon, after that we won't have any duplicity issue.

[1] https://github.com/Pajk/apipie-rails/issues/67

> 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).

Yes this is a problem which will be solved by creating API validations 
from model validations dynamically. We do not have to disable or stop 
using the API validations and lose its advantages.

> -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
>> |
>
> _______________________________________________
> katello-devel mailing list
> katello-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/katello-devel




More information about the katello-devel mailing list