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

Adds jsonld context, Adds support for jsonld typed values #752

Merged
merged 1 commit into from
Sep 30, 2016
Merged

Adds jsonld context, Adds support for jsonld typed values #752

merged 1 commit into from
Sep 30, 2016

Conversation

maechler
Copy link
Contributor

@maechler maechler commented Sep 12, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets -
License MIT
Doc PR api-platform/docs#122

This pull request is just an implementation proposal, it is not yet ready to be merged. I have not put any effort into fixing the tests.

This pull request wants to add json-ld typed values: https://www.w3.org/TR/json-ld/#typed-values
e.g.

entity

    /**
     * @var string | array
     *
     * @ApiProperty(iri="http://xmlns.com/foaf/0.1/firstName");
     *
     */
    private $firstName;

    /**
     * @var string | array
     *
     * @ApiProperty(iri="http://xmlns.com/foaf/0.1/lastName", valueType="literal");
     *
     */
    private $lastName;

context

{
    "@context": {
        "@vocab": "http://swissbib-hydra.lo/apidoc.jsonld#",
        "hydra": "http://www.w3.org/ns/hydra/core#",
        "firstName": "http://xmlns.com/foaf/0.1/firstName",
        "lastName": {
            "@id": "http://xmlns.com/foaf/0.1/lastName",
            "@type": "literal"
        }
    }
}

What do you think?

@@ -107,6 +107,11 @@ public function getResourceContext(string $resourceClass, int $referenceType = U
'@id' => $id,
'@type' => '@id',
];
} else if (null !== $propertyMetadata->getValueType()) {
Copy link
Member

Choose a reason for hiding this comment

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

Can be simplified using elseif.

@dunglas
Copy link
Member

dunglas commented Sep 12, 2016

@teohhanhui
Copy link
Contributor

What about having these as attributes, so that it can be JSON-LD specific, instead of trying to find a general enough "type" declaration that fits all the formats (sounds like an impossible task).

@maechler
Copy link
Contributor Author

I can also move it to the attributes. What would be the convention for the name?
Just use "value_type"="something" ?

@teohhanhui
Copy link
Contributor

There is already a hydra_context key, perhaps we can add jsonld_context?

@maechler
Copy link
Contributor Author

So it would be something like ?

    /**
     * @var string | array
     *
     * @ApiProperty(iri="http://xmlns.com/foaf/0.1/lastName", attributes={"jsonld_context": {"value_type":"literal"}})
     *
     */
    private $lastName;

@dunglas
Copy link
Member

dunglas commented Sep 13, 2016

Tyes, it looks better!

@teohhanhui
Copy link
Contributor

/**
 * @var string
 *
 * @ApiProperty(iri="http://xmlns.com/foaf/0.1/lastName", attributes={
 *     "jsonld_context"={"@type"="literal"},
 * })
 *
 */
private $lastName;

@maechler
Copy link
Contributor Author

Looks fine for me, I guess that's the way to go:)

@@ -97,6 +97,7 @@ public function getResourceContext(string $resourceClass, int $referenceType = U
}

$convertedName = $this->nameConverter ? $this->nameConverter->normalize($propertyName) : $propertyName;
$jsonldContext = $propertyMetadata->getAttributes()['jsonld_context'] ?? [];
Copy link
Contributor

Choose a reason for hiding this comment

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

What's set in the jsonld_context attribute should be used as the base. We don't want to limit it to just @type.

Copy link
Contributor

Choose a reason for hiding this comment

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

See for example how hydra_context is handled in

/**
* Gets and populates if applicable a Hydra operation.
*
* @param string $resourceClass
* @param ResourceMetadata $resourceMetadata
* @param string $operationName
* @param array $operation
* @param string $prefixedShortName
* @param bool $collection
*
* @return array
*/
private function getHydraOperation(string $resourceClass, ResourceMetadata $resourceMetadata, string $operationName, array $operation, string $prefixedShortName, bool $collection) : array
{
if ($collection) {
$method = $this->operationMethodResolver->getCollectionOperationMethod($resourceClass, $operationName);
} else {
$method = $this->operationMethodResolver->getItemOperationMethod($resourceClass, $operationName);
}
$hydraOperation = $operation['hydra_context'] ?? [];
$shortName = $resourceMetadata->getShortName();
if ('GET' === $method && $collection) {
$hydraOperation = [
'hydra:title' => sprintf('Retrieves the collection of %s resources.', $shortName),
'returns' => 'hydra:PagedCollection',
] + $hydraOperation;
} elseif ('GET' === $method) {
$hydraOperation = [
'hydra:title' => sprintf('Retrieves %s resource.', $shortName),
'returns' => $prefixedShortName,
] + $hydraOperation;
} elseif ('POST' === $method) {
$hydraOperation = [
'@type' => 'hydra:CreateResourceOperation',
'hydra:title' => sprintf('Creates a %s resource.', $shortName),
'returns' => $prefixedShortName,
'expects' => $prefixedShortName,
] + $hydraOperation;
} elseif ('PUT' === $method) {
$hydraOperation = [
'@type' => 'hydra:ReplaceResourceOperation',
'hydra:title' => sprintf('Replaces the %s resource.', $shortName),
'returns' => $prefixedShortName,
'expects' => $prefixedShortName,
] + $hydraOperation;
} elseif ('DELETE' === $method) {
$hydraOperation = [
'hydra:title' => sprintf('Deletes the %s resource.', $shortName),
'returns' => 'owl:Nothing',
] + $hydraOperation;
}
$hydraOperation['@type'] ?? $hydraOperation['@type'] = 'hydra:Operation';
$hydraOperation['hydra:method'] ?? $hydraOperation['hydra:method'] = $method;
if (!isset($hydraOperation['rdfs:label']) && isset($hydraOperation['hydra:title'])) {
$hydraOperation['rdfs:label'] = $hydraOperation['hydra:title'];
}
ksort($hydraOperation);
return $hydraOperation;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great input! I did not think of that, I updated the pull request.

@teohhanhui
Copy link
Contributor

Anyway, please add some tests. 😄

@maechler
Copy link
Contributor Author

I will add some tests, probably I'll need some time to have a look at the existing tests.

What about the docs, do you update them or shall I create a pull request there too?

'@id' => $id,
'@type' => '@id',
];
} else {
] + $jsonldContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

Reverse the order so that what's set in jsonld_context takes precedence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I thought in this case it might be better to force @id and @type. But the other way round it leaves more freedom for the developers. I'll change it.

@maechler
Copy link
Contributor Author

maechler commented Sep 19, 2016

Do I have to take care of the deps='low' build in that case?
It seems like it does not allow entities without ORM annotations.

@teohhanhui
Copy link
Contributor

This should also address #731

@albertedevigo What do you think?

@maechler
Copy link
Contributor Author

@teohhanhui What about the unit tests?

@maechler maechler changed the title Adds typed values Adds jsonld context, Adds support for jsonld typed values Sep 26, 2016
@teohhanhui
Copy link
Contributor

teohhanhui commented Sep 26, 2016

You should add a test for the JsonLd/ContextBuilder too, perhaps also covering the use case of #731?

@maechler
Copy link
Contributor Author

maechler commented Sep 27, 2016

@albertedevigo @teohhanhui I added a test for JsonLd/ContextBuilder.

I think #731 can be addressed with this issue. However I think there is also a need for a jsonld context on a resource level. I have created an issue here api-platform/api-platform#93 and now moved to #767.
That would also allow for the alternative use of @reverse described first in the issue.

@dunglas dunglas merged commit e122925 into api-platform:master Sep 30, 2016
@dunglas
Copy link
Member

dunglas commented Sep 30, 2016

Thank you @maechler! Can you open a doc PR on https://github.com/api-platform/docs/ to document this new feature please?

@maechler
Copy link
Contributor Author

maechler commented Oct 4, 2016

@dunglas Awesome, thanks!
Yes I will. Shall I add a new .md file to the core doc and an own documentation chapter "jsonld context"? I have not found any chapter where it would make sense to add this.

@dunglas
Copy link
Member

dunglas commented Oct 4, 2016

I think that it deserves a new chapter.

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
Adds jsonld context, Adds support for jsonld typed values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants