[katello-devel] Apipie and validation.

Dmitri Dolguikh dmitri at redhat.com
Mon Dec 10 17:55:51 UTC 2012 

On 10/12/12 04:09 PM, Petr Chalupa wrote:
> I'll try to explain better my reasoning.
>
> ## Facts I see:
>
> 1. As Bryan said we have to have API documentation.
> 2. API documentation is loosing value when it's outdated.
> 3. API request should be validated and a client should get readable 
> error when its format isn't right.

a nitpick: not just the format of data, but anything that renders the 
value invalid/illegal.

> 4. A client is talking to API.
> 5. Models should be hidden from client.

Don't like the phrasing. Resources != models; one model (or its parts) 
can be present in multiple resources.

>
> 6. Business logic should be captured and enforced by models.
> 7. An API request validation can be more complex or simpler than a 
> model validation. (complex example: batch update of resources; simple 
> example: update of a model where only a subset of attributes is exposed)

Agreed with the premise, but not with the conclusion. Batch of models is 
a model itself, governed by business rules, and should be modeled as 
such. A subset of attributes is still constrained by the business rules 
of the complete set of attributes.

A valid example is (pls. excuse the silliness): api exposes an attribute 
as Base64-encoded, while the model uses string. It's up to the 
controller to convert from one representation to another, and raise 
errors if that conversion fails. The controller doesn't look into the 
attribute itself to validate the value - that's model's job.

>
> 8. It's not a model responsibility to know how it's exposed via API.

Contollers define representation, composition, etc of data represented 
in models. Nothing else. They have nothing to do with validation of 
models - data supplied through API ultimately ends up in model. Model 
still needs to validate the data.

> 9. Duplication is bad.
>
> ## Implications:
>
> Points 5,6,7,8 implies that there should be another layer between 
> client and business logic, which is controllers. Controllers with help 
> of Apipie are defining the API.
>
> To avoid duplication (9) there has to be one source of information how 
> the API is specified (which is current done by Apipie's DSL in 
> controllers). (Models are not suitable for this because of 7. and 8.) 
> API specification is used both for generating API documentation and 
> request validation. That ties them together for documentation or 
> validation not to become outdated.

Once again, apipie is a documentation tool, which tries to be too many 
things. API (in rails world) is defined by the contents of routes.rb, 
nothing else. Validity of data is defined by model validators. Period.


>
> There is a duplication (9) between validation definition of modes and 
> API specification of request parts representing models. This can be 
> fixed by introducing helper to Apipie's DSL which will get the 
> information form models and reuse it for desired attributes.
>
> The example how the helper would look like:
>
> # in a controller
> api :POST, "/users", "Create an user"
> # use :only option to specify which attributes to use,
> # others are ignored
> param_by_model :user,
>                :only => [:disabled, :email, :password, :username,
>                          :default_environment_id] do
>   param :disabled, :bool # override default behavior if needed
> end
> def create
>   # ...
> end
>
> The helper only adds simple validations that: param is present, param 
> has correct type, param is not unknown. This ensures that models will 
> get correct hash of attributes, which I think is the main purpose to 
> ensure that model methods are called with valid data. More complex 
> validations are done by model.

Don't. Models can ensure they are populated with correct data. At best 
you are duplicating rules captured by the models. At worst you'll be 
introducing inconsistencies/bugs. Another thing to keep in mind that 
"simple" validations tend to turn into complex ones very quickly - the 
best thing we (api developers) can do is to return meaningful errors 
that point to the source of the problem.

>
> To comply with 3,4,5 ActiveRecord::RecordInvalid should be handled 
> same way as Apipie::ParamInvalid (easily doable by .rescue_from).
>
> ## Todo
>
> There are not many tasks left to do:
> - add the helper
> - report ActiveRecord::RecordInvalid in same way as Apipie::ParamInvalid
> - re-enable Apipie validation

Or, do nothing of the above and adopt the scrum solution to this 
organizational problem (pls. see below).

>
> ## Disadvantages of disabling Apipie validations
>
> - Outdated documentation unless it's periodically updated and checked

I have a suggestion: Add another completion criteria: no story is 
completed until docs have been updated.

> - less useful errors messages to a client

FUD. Model error messages can be as good as we need them to be.

> - duplication (API documentation vs model validations, and other 
> checks in controllers)

FUD, or plain wrong. Model can handle all the validations we need. 
apipie is the source of duplication in this case.

> - model errors are exposed to a client

Perfect! Remember, rails error messages point to attributes that are in 
error.

> - danger of adding another responsibilities to our models to handle this

It's not a danger - it's model's responsibility.



Again, now that the validation is off, it should stay off.
-d



>
> On 07.12.12 11:10, Dmitri Dolguikh wrote:
>> On 07/12/12 08:29 AM, Petr Chalupa wrote:
>>>
>>>
>>> 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.
>>
>> I have no clue what you mean by "validating API". At all. Nada.
>
> I mean validation of incoming JSON requests that they have correct 
> structure.
>
>>
>> Again: it's ok to validate cross-converns in controllers, those are
>> outside of model/business logic. Everything else is
>> duplicating/relocating business logic into the controllers.
>
> What I am talking about is not duplication or relocation, it's a 
> simple re-usage of information. Business logic would be still 
> described and enforced by models.
>
>> It's not apipie's responsibility to enforce validations. Drop this
>> functionality, or I'll be asking the team next to remove apipie
>> *completely*, as its developers are not being reasonable.
>
> This is only a cosmetic issue of placement. If you insist you can 
> extract API request validations to another rubygem.
>
>>
>>> 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.
>>
>> Don't we have more pressing stuff to do?
>>
>>>
>>> [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.
>>
>> This makes no sense. At best you'd be running model validations in
>> controllers. This is wasted effort, see my reasons above. Please stop
>> this madness.
>
> Please be patient, this discussion is not fun for me either, but I 
> would not continue in it unless I wasn't thinking that I have 
> something valuable to say. I tried to explain at the top.
>
>> -d
>>
>>>
>>>> -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
>>>
>>> _______________________________________________
>>> 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