[katello-devel] Apipie and validation.

Petr Chalupa pchalupa at redhat.com
Mon Dec 10 16:09:00 UTC 2012 

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.
4. A client is talking to API.
5. Models should be hidden from client.
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)
8. It's not a model responsibility to know how it's exposed via API.
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.

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.

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

## Disadvantages of disabling Apipie validations

- Outdated documentation unless it's periodically updated and checked
- less useful errors messages to a client
- duplication (API documentation vs model validations, and other checks 
in controllers)
- model errors are exposed to a client
- danger of adding another responsibilities to our models to handle this

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

More information about the katello-devel mailing list