Problem/Motivation

The Subrequests module provides replacement token functionality, so that data returned by a prior request can be used in a subsequent request.

For example, you may have one request to create a paragraph, and then a second to create a page that contains that paragraph. That second request requires the id and the revision id of the paragraph created in the first request. These are of types string and integer respectively.

Using jsonapi, this would require a subrequest of the following form:

  {
    "requestId": "page",
    "waitFor": ["paragraph"],
    "uri": "/jsonapi/node/article",
    "action": "create",
	"body": "{\"data\":{\"type\":\"node--article\",\"attributes\":{\"title\":\"Page created with json api and subrequests\"},\"relationships\":{\"field_sections\":{\"data\":[{\"type\":\"paragraph--wysiwyg_basic\",\"id\":\"{{paragraph.body@$.data.id}}\",\"meta\":{\"target_revision_id\":{{paragraph.body@$.data.attributes.drupal_internal__revision_id}}}}]}}}}",
    "headers": {
      "Accept": "application/vnd.api+json",
      "Content-Type": "application/vnd.api+json",
      "Authorization": "Basic ohhaithere=="
    }
  }

On replacement of the two tokens {{paragraph.body@$.data.id}} and {{paragraph.body@$.data.attributes.drupal_internal__revision_id}} this would become valid JSON.

However, before token replacement, this is not valid JSON because {{paragraph.body@$.data.attributes.drupal_internal__revision_id}} contains non-numeric characters and is not quoted.

JsonBlueprintDenormalizer::fillDefaults() does $raw_item['body'] = Json::decode($raw_item['body']);. Because our non-quoted token isn't valid JSON, this returns null, and further down the stack an exception is thrown, and this subrequest fails.

Note in this example we can work around this by quoting the token and pretending it is a string and letting PHP magic convert it as appropriate. But this is a fudge and would fall down with boolean values. Ultimately introduction of tokens means subrequests aren't actually JSON, so we shouldn't assume they are.

Proposed resolution

Don't parse non-JSON as if it's JSON.

Remaining tasks

All the things.

User interface changes

None.

API changes

Hmmm.

Data model changes

None.

Release notes snippet

✄

CommentFileSizeAuthor
#12 3048493--interdiff--11-12.txt974 bytese0ipso
#12 3048493--add-bool-handling-patch--12.patch3.77 KBe0ipso
#11 3048493_11_add_bool_handling.patch2.82 KBEli-T
#8 3048493_8.patch2.76 KBEli-T
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Eli-T created an issue. See original summary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

Thanks for contributing!

But this is a fudge and would fall down with boolean values.

This is true. It's far from ideal, but a workaround for booleans is "0" and "1".

Ultimately introduction of tokens means subrequests aren't actually JSON, so we shouldn't assume they are.

I don't agree with this statement. I think that the token is a string. The problem is that the calculated replacement is always treated as a string, regardless of the actual type (do you agree?). The replacement may or may not need type casting, and that's the actual problem. Maybe something like \"id\":\"{{paragraph.body@$.data.id:int}}\" would do. Or maybe it's unnecessary and we can detect the type from the previous subresponse during replacement time.

I'm leaning towards the second.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented

Thanks for the rapid response, and all your work on this module!

I think that the token is a string.

I think that's fine if it's explicitly stated that the token must always be wrapped in quotes and those quotes can actually be replaced; effectively this makes the quotes as much part of the token as the surrounding pairs of braces.

Or maybe it's unnecessary and we can detect the type from the previous subresponse during replacement time.

If possible that seems far more flexible and elegant than embedding type data into the subrequest.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

Glad to see we are on the same page. Any chances you can give this a try?

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented

I'll take a look this afternoon :)

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented

When the token replacement is actually being performed in JsonPathReplacer::replaceTokenSubject($token, $value, $token_subject), $value is passed in as int if that's what the token is when retrieved from the source.

So at that point, before we do the preg_replace(), we could check if $value is *not* a string, and if so, first attempt to replace with $token wrapped in double quotes.

But it's entirely possible that a token representing a non-string is embedded within a string. EG 

  "arbitrary thing": "blah/{{paragraph.body@$.data.attributes.drupal_internal__revision_id}}"

so we'd have to also try to preg_replace() without wrapping in quotes to find such occurrences.

At that point, we can't tell if the intention of

"arbitrary thing": "{{paragraph.body@$.data.attributes.drupal_internal__revision_id}}"

in the subrequest is to embed the integer in the string that happens to have nothing else in it, or whether it should be replaced as a JSON integer.

So I'm not sure approach 2 in #2 is actually possible 🙁

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

Luckily there are not many scalar types in JSON. From json.org

A value can be a string in double quotes, or a number, or true or false or null, or an object or an array. These structures can be nested.

We can remove the quotes during the replace if:

  1. The replacement value is number, true, false or null
  2. The token is the only thing in the JSON value.

That would solve the issue, right?

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
FileSize
3048493_8.patch2.76 KB

Work in progress, with tests.

This works with everything except true/false, as they are not passed in to replaceTokenSubject() as PHP booleans.

We'd also need to change the message in the exception thrown in validateJsonPathReplacements() as we are not expecting a list of strings anymore.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented

(also need to remove the type hint for replaceTokenSubject() $value parameter)

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented

Also JsonPathReplacerTest::testReplaceBatchTypes() has a var_dump() that needs removing.

Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented
Status: Active » Needs review
FileSize
3048493_11_add_bool_handling.patch2.82 KB

Fixed bool handling and removed var_dump() mentioned in #10.

Probably a good time for a review :)

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
FileSize
3048493--add-bool-handling-patch--12.patch3.77 KB
3048493--interdiff--11-12.txt974 bytes

Well, all versions prior to 5 of the schema validator have vanished. I'm curious about what happened.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
Status: Needs review » Fixed

Tests are passing locally. Merging.

  • e0ipso committed a07be20 on 8.x-2.x authored by Eli-T 
    Issue #3048493 by Eli-T, e0ipso: JSON data types other than string can't...
Eli-T’s picture

Eli-T
he/him
Primary language English
Location Manchester
CreditAttribution: Eli-T commented

Awesome! I'll raise a follow up to address the change to the message in the exception thrown in validateJsonPathReplacements() as we are not expecting a list of strings anymore, as mentioned in #8. But tomorrow :)

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

I'll raise a follow up to address the change to the message in the exception thrown in validateJsonPathReplacements() as we are not expecting a list of strings anymore, as mentioned in #8. But tomorrow :)

Fantastic! I wanted to get this committed soon. I agree, we can address that in a follow up.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.