Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
When Schemata is present, we can generate schemas for our resources. That means that we can validate that all requests coming out of JSON API can be validated against those schemas.
We would like to use the same strategy for that we are using to validate agains the generic JSON API schema.
Comment | File | Size | Author |
---|---|---|---|
#62 | 2917260--validate-against-specific-schemas--62.patch | 5.63 KB | e0ipso |
#59 | 2917260--validate-against-specific-schemas--59.patch | 5.64 KB | gabesullice |
#59 | interdiff--2917260--57-59.txt | 796 bytes | gabesullice |
#57 | interdiff-51-57.txt | 3.72 KB | gabesullice |
#57 | 2917260--validate-against-specific-schemas--57.patch | 5.63 KB | gabesullice |
Comments
Comment #2
e0ipsoThis is the initial pass. Please review.
This already uncovered some issues with the schema generation:
- If JSON API Extras disables some fields that are marked as required, then Schemata should drop them from the
required
section.- If JSON API Extras customizes names of fields that are marked as required, then Schemata should add the custom name in the
required
section.Comment #4
Wim LeersComment #5
Wim Leers+1 for principle!
I think all this new logic should be moved to a separate helper method. The code is currently hard to read.
Comment #6
dawehnerI'm wondering whether we can do some profiling to see how much costs are included on every page response, when you have schemata enabled. I fear people would run those modules on production.
Comment #7
Wim LeersBut
assert()
hopefully is not enabled on production servers.Comment #8
dawehnerOh nevermind :) I missed that this function is called from assert().
Comment #9
Wim LeersHeh, np :)
First, let's make this pass tests.
Comment #10
Wim LeersComment #12
Wim LeersForgot to include the updated service definition hunk.
Comment #13
Wim LeersI get this error:
… until I install
schemata_json_schema
. i.e. just theschemata
module is not enough.So let's make that a test dependency.
Comment #15
Wim LeersForgot to update the test expectation.
Comment #16
Wim LeersIt looks like testbot doesn't actually respect
test_dependencies
… so let's also add it tocomposer.json
'srequire-dev
too. Fingers crossed 🤞Comment #17
Wim LeersLooks like that worked:
Now I still need to install that module during tests. I know where to continue on Monday now! :)
Comment #18
e0ipsoThat's a nice workaround with the require-dev.
$this->validateSchema($specific_schema, $resource_object) && $valid;
Is it worth to reverse this && so we skip faster when
$valid
is false?Comment #19
Wim Leers#2920892: Remove all assert('string') calls from JSON API because deprecated in PHP 7.2 landed and conflicts with this, rebased.
Comment #20
Wim Leers#18: I've wondered about that exact same thing, and I even changed that line :D But then I figured you had done it this way to ensure everything would be validated, i.e. that we wouldn't not validate the second entity in a collection just because the first was invalid. Which is why I removed that from my review.
Comment #21
e0ipsoWe should bail on checking the schema for the
related
andrelationship
routes as well.Comment #22
Wim LeersAdded this to #2842148: The path for JSON API to "super stability".
#21: why? Because the JSON Schema cannot possibly cover that, right?
Comment #23
Wim LeersComment #24
Wim LeersMarked #2917260: Validate against specific JSON Schemas if Schemata is present as a duplicate.
Comment #25
e0ipsoI agree with @Wim Leers that this is a high priority issue, even though it may not be the most pressing for core's inclusion.
Comment #26
Wim LeersIt's not pressing for core inclusion directly, but it is indirectly, because it proves there is a certain level of accuracy/robustness. Which means it's easier for core committers to trust that things are working as intended.
In other words: this means there's less of a need for core committers to understand technical implementation details, because they'll be able to read the schema instead, observe that it makes sense, and since tests are passing, that means that all the code in the JSON API module is in fact generating responses matching that schema!
Comment #27
e0ipsoYep, nevertheless I think this needs to be worked on soon. I may take a stab at it soon if you or Gabe don't beat me to it.
Comment #28
Wim Leers+1. I know Gabe said he was interested in working on this, but I'm sure he wouldn't mind you working on it instead!
Comment #29
e0ipsoI will leave this to Gabe then.
Comment #30
gabesulliceI checked with @e0ipso (via drupal.slack.com #jsonapi) to find out what was remaining on this issue. His summary of the remaining work was as follows:
Comment #31
gabesulliceThis patch addresses numbers 2 and 3 from #30.
I think we have to rely on assert being disabled in production. I can't think of any other check to perform. It looks like that was the opinion expressed in #7 as well.
The tests in this patch should show that validation is not performed when asserts are disabled... but only in PHP 7.1 and above.
Unfortunately, these tests will fail in PHP 5.x because validation will still run (see #2920892: Remove all assert('string') calls from JSON API because deprecated in PHP 7.2 for why we can't make these pass).
I haven't addressed number 1 yet.
@e0ipso, I think we still want the generic schema validations to run for related/relationships, correct?
Comment #32
e0ipsoYes. I would say so.
Isn't there any way to prevent tests from running in 5.5, 5.6 and 7.0?
Comment #33
gabesulliceYes. I wasn't very clear. I meant, "the particular tests on this comment" will fail and we can't stop PHP from doing validation.
I plan on doing a PHP version check to ensure we get passing tests. I just wanted it to be clear on the fact that there's difference between 5 & 7.
Comment #34
gabesulliceAs promised, PHP < 7 tests are skipped.
Comment #35
gabesulliceThe attached now addresses #1 from #30, cleans up the tests, comments, etc.
Comment #36
e0ipsoHere's the first review. Only some very minor comments.
Pretty cool! 🆒
This is probably a leftover.
Is this code path exercised anywhere?
Should we make a service out of
Validator
and inject it instead?This can only be
TRUE
at this point, right?Should we return
TRUE
instead for clarity?This doesn't feel great. I don't think we have any better way.
Does anyone feel that we should add a route option and check for that here instead of string matching on a string?
@gabesullice can you create a follow up?
Same comment as above.
Maybe
Json::decode
? Or we have that problem on coercing arrays instead of stdClass objects?Just making sure.
Comment #37
e0ipsoWe agreed on #2920892-13: Remove all assert('string') calls from JSON API because deprecated in PHP 7.2 to only execute validation if the assertions are enabled. Even in PHP 5.5 and 5.6. We never ended up creating that issue 😑, but I think this is the moment to implement that. We are harming performance in lower PHP versions (which are already performance impared).
Comment #38
gabesulliceI thought about doing this and researched it, but couldn't figure out how to make a service out of it only when the library exists. We could make a service _and_ a factory, but then we'd still end up with similar logic trying to find out if the service was actually injected or not.
TL;DR: it would be a whole lot of work and complexity for the same result, but maybe I'm missing a fancy trick.
Heh, I actually switched it to this because I thought this way was more clear. Sure, only one thing is possible, but this communicates that we're "falling back" to the generic result from above.
I agree that this isn't ideal, but I don't think there's a better way either. I also don't feel like a route option is any better because we're kind of abusing the routing system with those already. I've love to see something like "route tags" introduced, but for now, appending the name feels like the cleanest way to do that.
Hm, not sure. Thats's just how it was. I'll give it a try.
Comment #39
e0ipsoLet's switch it back to
TRUE
and let's add a one line comment for clarity.What is your concern about the suggested (and already current) approach?
Comment #40
gabesulliceRemoved.
Yes, that's how the Validator mocks are injected. See
testDoValidateResponse
.Will do. Let's discuss there.
Done.
Oui Chef! ;)
Re: #37: Only run asserts when asserts are active.
Done. Good suggestion, I like this approach better than skipping tests.
Comment #41
gabesulliceFollow up to #36.6: #2932679: Remove unused "on_relationship" serialization context
Comment #42
e0ipsoThis issue is pretty awesome! I'm excited to ship this feature!
Comment #43
e0ipsoComment #45
gabesullice\o/
Comment #46
e0ipsoReopening due to an interaction with xdebug on PHP 7.1
Attached patch.
Comment #47
e0ipsoComment #49
e0ipsoFixed tests
Comment #50
e0ipsoComment #51
e0ipsoComment #52
gabesulliceThanks for alphabetizing XD
Nice!
Are we sure this won't have unforeseen, negative effects?
If the answer to #3 is "yes," let's get this merged.
Comment #53
e0ipsoI wondered that myself. But since tests pass and the constant was not being declared inside the test, I'm reasonably confident this should not be an issue. Maybe Daniel can clarify.
Comment #54
dawehnerIn tests there is
$this->root
you can use directly, which you can inject using$container->get('app.root')
.Does that make sense?
Comment #55
Wim Leers#40+#44: wow, I need to be away more often, great stuff like this just is already committed when I get back, and then you trick me by having the issue marked
, leading me to think it's not yet committed 😀Redeclaring core constants definitely is smelly, and #54 is better. Yes, there still are some core tests where this happens. But not many. Most of those cases were removed. Precisely because doing this caused so much problems :) So, NW++ to implement @dawehner's suggestion.
Comment #56
gabesulliceI'll finish this one up. @e0ipso or @Wim Leers, just ping me in slack if you're already been working on it.
Comment #57
gabesulliceThanks for the tip @dawehner!
Comment #59
gabesulliceDumb mistake...
Comment #60
Wim LeersComment #61
e0ipsoThis should say:
protected $appRoot;
Comment #62
e0ipso