Skip to content
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

[OS-17] JSON Schema validation #45

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Marahin
Copy link
Collaborator

@Marahin Marahin commented Aug 25, 2020

Unknown(s), to debate(s), discussables:

  • Where should we raise JsonApi::Parameters translator errors? In which cases? This is important, because if we do not raise an error, then in the default behaviour - no validation will happen.
  • How to handle JSON Schema invalidation / updates?

spec/support/jsonapi_schema.json Outdated Show resolved Hide resolved
lib/jsonapi_parameters/validator.rb Show resolved Hide resolved
lib/jsonapi_parameters/validator.rb Show resolved Hide resolved
lib/jsonapi_parameters/validator.rb Outdated Show resolved Hide resolved
lib/jsonapi_parameters/parameters.rb Outdated Show resolved Hide resolved
spec/support/jsonapi_schema.json Outdated Show resolved Hide resolved
@Marahin Marahin requested a review from a team August 26, 2020 14:13
README.md Outdated Show resolved Hide resolved
Co-authored-by: Patryk Ptasiński <[email protected]>
@Marahin
Copy link
Collaborator Author

Marahin commented Oct 1, 2020

Things left to do:

  • make the schema more liberal for root path keys OR handle removal of action, controller and other Rails-specific middleware parameters that are added during the request cycle to the ActionController::Parameters object instance (right now they are validated as well and the schema makes a note about it that these are not recognized or expected keys),
  • find out how to handle this as a major or minor (non-breaking) version

@Marahin Marahin force-pushed the os-17/json-schema-validation branch from 035acc5 to e9f8b7b Compare October 2, 2020 13:39
@ipepe
Copy link
Member

ipepe commented Oct 28, 2020

The current version works "good enough" for us at LL, there are exactly 3 tests failing and they are just nontypical usage of jsonapi-parameters . Our recommendation is for the current version to be merged but with at least the "MINOR" version increase

@Marahin
Copy link
Collaborator Author

Marahin commented Oct 29, 2020

@ipepe I'm wondering whether other users may see a breaking change while updating. If I understand correctly, you are experiencing this, so this would imply a breaking change version bump.

Could you share (perhaps in a confidential environment) what are the non-typical usages? Specific code lines where you execute jsonapi_parameters translation?

Maybe this way we could provide an interface that would not enforce the change so the existing users would be able to take control over the update.

@@ -0,0 +1,397 @@
{
"$schema": "http://json-schema.org/draft-06/schema#",
Copy link
Contributor

Choose a reason for hiding this comment

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

there is draft -07

Copy link
Collaborator Author

@Marahin Marahin Jan 29, 2021

Choose a reason for hiding this comment

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

Even now 07 is not edge: http://json-schema.org/draft/2019-09/release-notes.html

I believe we should resolve the subject of version update (and whether it can be minor upgrade) in our library and then we can think about moving on to next version of schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://json-schema.org/draft/2019-09/schema
that's right. Therefore the draft was upgraded also in article linked in repo 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

davishmcclurg/json_schemer#44
is supporting max draft-07 currently according to readme

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Therefore the draft was upgraded also in article linked in repo 👍

I have no clue which article or link you are talking about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

end
@jsonapi_unsafe_hash = ensure_naming(params, naming_convention)

JsonApi::Parameters::Validator.new(@jsonapi_unsafe_hash.deep_dup).validate! if should_prevalidate?
Copy link
Contributor

Choose a reason for hiding this comment

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

validate! has been provided by rails >= 5.0
So this solution limits the benefits for rails 4.0, which are not maintained anymore.
Michał Książek is coding solution to change that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That should be pretty easy to overcome, as #validate is present since Rails 4.2.1: https://apidock.com/rails/v4.2.1/ActiveModel/Validations/validate

So perhaps it is worth to check whether this method is defined, and if not, then just call validate and raise on failure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since we already limit ourselves to >= 4.1.8, we might as well limit ourselves to 4.2.1.

https://github.com/visualitypl/jsonapi_parameters/blob/master/jsonapi_parameters.gemspec#L25

Copy link
Contributor

Choose a reason for hiding this comment

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

We spoke with Michał to drop activeModel validation at all - it does not provide big value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed from what I can see in the upcoming PR to this branch, this seems like a move in good direction. We just have to ensure this will be a solid solution, as this will be something technically "invented here" whereas up until this point we have used proven and well tested ActiveModel validations.

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.

4 participants