Skip to content

Initial commit for a possible replacement of the parser. #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Oct 2, 2016

Conversation

beauby
Copy link
Owner

@beauby beauby commented Oct 1, 2016

See README.md for more info. Basically, validate a document without creating a bunch of objects. The object-like API for JSONAPI documents disappears, but some domain-specific validation directives are added (this functionality has been moved to an external gem: jsonapi-validations).
Some general-purpose domain-specific validation directives for general documents may be added later. (would happen in jsonapi-validator)

@beauby beauby force-pushed the separate-parser-validator branch 2 times, most recently from c0ccac8 to 3a799b6 Compare October 1, 2016 01:41
@beauby beauby force-pushed the separate-parser-validator branch from 247af8b to 613906b Compare October 1, 2016 01:54
Copy link

@richmolj richmolj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't gone through every line but wanted to give feedback on the things that jumped out to me.

Document.ensure!(document.is_a?(Hash),
'A JSON object MUST be at the root of every JSONAPI ' \
'request and response containing data.')
Document.ensure!(document.key?('data') || !document['data'].is_a?(Hash),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe data can be an Array when POSTing to a relationship endpoint http://jsonapi.org/format/#crud-updating-to-many-relationships

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that case is handled in relationship.rb. C.f. this comment.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I gotcha, this is smart 👍


module JSONAPI
module Validator
class Resource
Copy link

@richmolj richmolj Oct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would personally prefer this to live in a separate gem, strong_parameters, something new, whatever. The rest of the gem is focused on generic, low-level jsonapi handling, this introduces a higher-level domain-specific usage that should live one level higher IMO.

I could also see this growing in complexity with type validation/coercion, rule matching, etc.

Copy link
Owner Author

@beauby beauby Oct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your concerns, and partly share them. Here are the motivations though:

  1. Validating a JSONAPI document should be as fast as possible, hence instantiating a lot of objects (as jsonapi-parser does) is a problem;
  2. Validating a JSONAPI document and providing an object-like API to traverse it are two different concerns that should be separated;
  3. The JSONAPI spec seems to define "one format" but what it really defines is three: one for response documents, one for resource creation/update payloads, and one for relationship update payloads. The latter two being somewhat narrower than the former, although the second allows absence of id for the primary data where the first does not.
  4. This code is about validating the syntax of a document, and presence/absence of some attributes/relationships, as well as ensuring the value of some type members arguably falls within syntactic analysis, although this part is clearly domain-specific. This is the concern that I share with you, and I'd like to find an elegant solution to separate those.
  5. I tend to find little value to the parser outside of validation. Maybe it could encapsulate more responsibility but I'm not sure what kind would be useful at this point.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the following:

  1. The current validator, without the whitelisting/type checking becomes what is currently called the parser;
  2. validator becomes an external gem.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we agree on all points, actually.

Regarding 5 - just spitballing here, but I could see it as a low-level tool for spec helpers that need to traverse and validate sections of a jsonapi document. My jsonapi_spec_helpers gem could use it.

Regarding the separate validator gem - no disagreement, but curious on the overall direction/usage you expect of it. Current alternatives like sinatra-param and strong_parameters no good?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sinatra-param and strong_parameters very good for rails, but the dependency on actionpack is not in line with the philosophy of this collection of gems.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well sinatra-param doesn't have an actionpack dependency, and there are others such as json-schema.

My point is: this logic usually comes after deserialization, which means it's "one level up" and not jsonapi-specific. At that point there should be libraries already serving this purpose. Why do we need another one specific to jsonapi?

Once again, not against it at all just want to get a better understanding of the fuller picture.

Copy link
Owner Author

@beauby beauby Oct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sinatra-param has a Sinatra dep, which is the same problem.
The idea is that the basic validation can happen in the jsonapi document directly, so that it can then be safely transformed into an arbitrary structure, rather than making a first transformation for convenience and then re-transforming it into something useful, which is wasteful. I'm not saying it's the best approach but it's currently the one that makes most sense to me.

Copy link
Owner Author

@beauby beauby Oct 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is also motivated by the fact that when deserializing to an intermediate structure, you are usually left with doing the exact same validation work for building your custom structures as you would have done directly on the payload itself.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it's less about gem dependencies and more about a wasteful intermediate step then I'm with you. I would personally choose to keep the wastefulness and use some already-existing gems, but I can see how this would be less than ideal and others would prefer to solve the root problem. If nothing else, I think the conversation has re-emphasized making this a separate gem was a good step to take 👍

Parser. The previous parser goes out of the picture.
@beauby beauby force-pushed the separate-parser-validator branch from c83f118 to 69cb76b Compare October 1, 2016 17:50
@beauby beauby force-pushed the separate-parser-validator branch from 7361ec2 to 4f9ab8f Compare October 2, 2016 15:55
@beauby beauby merged commit 3db136e into master Oct 2, 2016
@beauby beauby deleted the separate-parser-validator branch October 2, 2016 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants