Use ContextGenerator for JSON serializer#23
Conversation
|
🤦♂️ tests! |
Also update tests
| public function __construct(LinkManagerInterface $link_manager, | ||
| EntityResolverInterface $entity_Resolver, | ||
| JsonldContextGeneratorInterface $jsonld_context) { | ||
| parent::__construct($jsonld_context); |
There was a problem hiding this comment.
why the parent::__construct($jsonld_context)?
There was a problem hiding this comment.
Because the $this->jsonldContextGenerator is initialized in the FieldItemNormalizer which this class extends. So that variable is already handled in the parent class constructor.
| $field->getFieldDefinition(), | ||
| $context['namespaces'] | ||
| ); | ||
| $values_clean = $values_clean + $field_context[$field_keys[0]]; |
There was a problem hiding this comment.
Is this key always populated?
|
@whikloj This looks good from what I can make of it. I'm going to give it a go with a new vagrant and see what happens. |
|
@whikloj I'm getting this when attempting to rebuild the router after pulling in your changes: |
|
@dannylamb did you clear caches? Service YAML file gets cached in Database https://www.drupal.org/node/2540430, so probably after you got this code it could be still using the old one. Looking through the rest of the code anyway now, but first look seems fine, https://github.com/Islandora-CLAW/jsonld/pull/23/files#diff-59b468be4869b82b42122d98a064f7bbR4 is referencing the right Service, well named, and that service is of a class that extends |
|
@DiegoPino I was getting that error when trying to either rebuild the router or clear caches. Did you manage to get it running when reviewing previously? If all else fails maybe I have to do a full uninstall of our modules, then pull in the code and reinstall all the modules. |
|
@whikloj So there's issues uninstalling the main islandora module, but that's totally independent of this PR. Nonetheless, I did manage to uninstall and re-install everything and that obliterated whatever was getting me in a "cache 22". Unfortunately, now I'm getting this when serializing out to jsonld: |
| $context['namespaces'] | ||
| ); | ||
| if (isset($field_context[$field_keys[0]])) { | ||
| $values_clean = $values_clean + $field_context[$field_keys[0]]; |
There was a problem hiding this comment.
If one of the vars at line 114 here is not an array it will fail via
PHP Fatal error: Unsupported operand types. This is what @dannylamb is seeing. Which means we need to make sure that everything is A) and array or cast. Not sure about the cast because it can lead to undesired results.
|
Adding @dannylamb Can you explain what type of object you were getting your error on? I can add it as a test case. |
|
@whikloj this test is failing |
|
Oops, didn't a) run the tests and b) missed that array is already set in one. |
|
😡 I spend more time destroying vagrant machines because Drupal gets stuck unable to run with a module and at the same time unable to uninstall that module. |
|
@Islandora-CLAW/committers Ok, I pulled and built a new vagrant and then immediately did this. Gaa? Ideas? |
|
I've had this happen before. Sometimes our configs in |
|
Ok, I manually deleted the TN field configs and then reinstalled this PR along with the same islandora, islandora_collection and islandora_image. It worked. So perhaps Islandora/documentation#685 needs to be a priority |
|
@whikloj So the document we're sending to fedora looks like this: [
{
"@id":"",
"@type":[
"http:\/\/pcdm.org\/models#Object",
"http:\/\/schema.org\/ImageObject"
],
"http:\/\/purl.org\/dc\/terms\/title":[
{
"@value":"Ooo-be-doo",
"@type":"http:\/\/www.w3.org\/2001\/XMLSchema#string"
}
],
"http:\/\/schema.org\/author":[
{
"@type":"@id",
"@id":"http:\/\/localhost:8000\/user\/1?_format=jsonld"
}
],
"http:\/\/schema.org\/dateCreated":[
{
"@value":"2017-08-04T17:38:05+00:00",
"@type":"http:\/\/www.w3.org\/2001\/XMLSchema#dateTime"
}
],
"http:\/\/schema.org\/dateModified":[
{
"@value":"2017-08-04T17:38:46+00:00",
"@type":"http:\/\/www.w3.org\/2001\/XMLSchema#dateTime"
}
],
"http:\/\/purl.org\/dc\/terms\/description":[
{
"@value":"I wanna be like you-oo-oo.",
"@type":"http:\/\/www.w3.org\/2001\/XMLSchema#string"
}
],
"http:\/\/pcdm.org\/models#hasFile":[
{
"@type":"@id",
"@id":"http:\/\/localhost:8000\/media\/1?_format=jsonld"
},
{
"@type":"@id",
"@id":"http:\/\/localhost:8000\/media\/2?_format=jsonld"
}
]
}
]It's this segment that's causing the problem: "http:\/\/schema.org\/author":[
{
"@type":"@id",
"@id":"http:\/\/localhost:8000\/user\/1?_format=jsonld"
}
],Fedora is returning 400 with |
|
@whikloj So I'm guessing Fedora is interpreting everything with a |
|
Hrm... and stripping out the author bit makes it blow up with the same error on the @whikloj I'm thinking we may have to drop the |
|
@whikloj Bump, we can't have |
|
Umm no I think entity references can be different. I'll have to get back into it and have another look. |
|
@dannylamb oh wait, I didn't fully think through your statement... I'm just wondering if (and I know this is more work) we should investigate this on the Fedora side as well. It seems weird that it requires the object to exist in the repository if we say the type is an id. Especially if this is the object of a triple and so therefore could be anywhere in the world. |
This. Six hundred and twelve times this. The "integrity constraint" here is a pure artifact of the entanglement between RDF types and Modeshape node types in the Fedora/LDP impl under discussion. It's hard to imagine that any other system would show it. |
|
I just did a test PUT into Fedora built off master with the following and it worked. |
|
I share @DiegoPino's concern about whether |
|
Just to stick my nose in here, but you should be using |
|
@ajs6f the use of |
|
I was able to PUT the following, so perhaps it is the format we are sending to Fedora? |
|
@whikloj: it is likely due to the fact that Fedora is using really old RDF processing libraries. The 4.7.x branch uses a patched/forked (old) version of Jena, and so updating that will be pretty painful, but updating that is also out of the question b/c of the RDF 1.0 vs RDF 1.1 issue from a year ago (yuck!). The master branch has a slightly more modern Jena (and it doesn't use a forked version of the library), so it's not surprising that it would do better. |
|
@acoburn You sure? Are you sure that the Modeshape impl just doesn't just fail to deal properly with non-repo RDF type URIs? |
|
Oh, this isn't connected to RDF type? If not then I am throwing dirt in the air, sorry. |
|
Ok, here are the 5 tests I tried against Fedora. 2 failed, 3 succeeded but one of those was using |
|
@whikloj were those tests applied to Fedora 4.7 or against master? |
|
@whikloj which ones are alternative/by spec transformations via the JSON-LD playground and which ones are modified manually. Thanks a lot Jared, I feel that is what we needed |
|
@acoburn this was against master (specifically fcrepo/fcrepo@26c64ae) @DiegoPino I think 3, 4, & 5 were alternatives, but I validated them all using the JSON-LD playground site. |
|
@whikloj: I say all of this just by looking at the JSON-LD (without doing any validation) so take this w/ a grain of salt.
|
|
@acoburn w/r/t |
|
@whikloj I'm sure it's correct on the JSON-LD playground -- it's just a syntax that I'm not familiar with. In fact, I generally try to avoid expanded JSON-LD; compact JSON-LD is much nicer to deal with. |
|
Ok, so I'm going to take Succeeded_4.json from the above Gist and make that my test's expected outcome and work towards that. If there is any reason you can see for not doing that, please let me know. |
|
@whikloj go for it. I'm too deep into release process today but i promise to do research in the next days on possible issues and edge cases and come back and comment on any changes you make. Thanks a lot for this. Update: Seems like Succeeded_4.json requires just the removal of |
|
Ok so question. @dannylamb you said here that the document being sent to Fedora had that format. How did you determine that? Because the format in my tests included the referenced entity (like the Author or File). I was going to remove that as that causes a problem for Fedora (trying to dereference those out of repo URIs), but your comment doesn't have the extra referenced entity graphs? |
|
@whikloj Author is referenced in that resource. Not sure what you mean. I tested by going to the |
|
@dannylamb So by default we embed the other resources in the same JSON-LD document. So if you have a node with a JPG in it the JSON-LD contains both the graph for the node (which references the media entity), but also a graph entry for the media entry. That is what I see from the output in the tests (ie. this exists in the output to dereference this). But your example doesn't have those extra bits in the JSON-LD, so are you removing them? I ask because I was wondering if we should add an option to allow us to not embed the referenced entity in the output JSON-LD if we so choose. All we need to do (I think) is change this line to We could do that based on either:
|
|
@whikloj Yes, I'm stripping out the I think ideally it'd accept a header or query param and the user can set the default value of that in configuration. That way Milliner can always request it the same way and we can program against just one of the options while still giving people flexibility. For this issue, I say pick whatever output you think is best and I can help adapt Milliner if need be. Then go ahead and make another issue for making the serializer and REST resource work with headers/params and be user configurable because we're scope creeping pretty hard right now. |
|
FYI, there's already a mechanism to do this sort of thing through for requesting expanded/compacted foms through the Accept header (not that Drupal even respects it): https://www.w3.org/TR/json-ld/#application-ld-json |
dannylamb
left a comment
dannylamb
left a comment
There was a problem hiding this comment.
@whikloj++
I've tested the most recent changes and they don't break Fedora sync'ing.
@DiegoPino Can you release your block / merge this if all your feedback has been addressed?
|
@whikloj++ @DiegoPino++ |
|
All credits to @whikloj. Good work =) |
5 participants
GitHub Issue: Islandora/documentation#661
What does this Pull Request do?
Uses Json-ld Context Generator for normalizer.
What's new?
(ie. Regeneration activity, etc.)? no
How should this be tested?
cd /var/www/html/drupal/web(for vagrant)drupal router:rebuilddrupal cache:rebuild all@typeproperties.Additional Notes:
Any additional information that you think would be helpful when reviewing this
PR.
Interested parties
@DiegoPino @dannylamb @ajs6f