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

Persisting an entity with a ManyToOne relation does not convert related entity as IRI #46

Closed
jmfeurprier opened this issue Jan 27, 2017 · 10 comments

Comments

@jmfeurprier
Copy link
Contributor

jmfeurprier commented Jan 27, 2017

We have the following entity classes:

<?php

namespace AppBundle\Entity;

use Mapado\RestClientSdk\Mapping\Annotations as Rest;

/**
 * @Rest\Entity(key="articles")
 */
class Article
{
    /**
     * @Rest\Id
     * @Rest\Attribute(name="id", type="string")
     */
    private $id;

    /**
     * @Rest\ManyToOne(name="section", targetEntity="Section")
     */
    private $section;

    public function setId($id)
    {
        $this->id = $id;
    }

    public function getId()
    {
        return $this->id;
    }

    public function setSection(Section $section)
    {
        $this->section = $section;
    }

    public function getSection()
    {
        return $this->section;
    }
}

and

<?php

namespace AppBundle\Entity;

use Mapado\RestClientSdk\Mapping\Annotations as Rest;

/**
 * @Rest\Entity(key="sections")
 */
class Section
{
    /**
     * @Rest\Id
     * @Rest\Attribute(name="id", type="string")
     */
    private $id;

    /**
     * @var string
     *
     * @Rest\Attribute(name="title", type="string")
     */
    private $title;

    public function setId($id)
    {
        $this->id = $id;
    }

    public function getId()
    {
        return $this->id;
    }

    public function setTitle($title)
    {
        $this->title = $title;
    }

    public function getTitle()
    {
        return $this->title;
    }
}

When we persist an entity like this:

// $section is a valid and existing entity fetched from the API.

$article = new Article();
$article->setSection($section);

$articleRepository = $sdk->getRepository('articles');
$articleRepository->persist($article);

Then the HTTP call to the API is like this:

Path: POST http://domain.tld/articles

Parameters: { "json": { "section": "946c3a2a-e36a-11e6-b8b0-0242ac110002", "title": "test" }, "version": "1.0", "headers": { "Accept-Language": "en-US,en;q=0.5" } } 

Notice how the "section" key is "946c3a2a-e36a-11e6-b8b0-0242ac110002" instead of "/sections/946c3a2a-e36a-11e6-b8b0-0242ac110002"

I think the fix could be to modify the Serializer::recursiveSerialize method, around line 189, by replacing:

if ($data->getId()) {
    $data = $data->getId();
} elseif (...) {

with:

if ($data->getId()) {
    $hydrator = $this->sdk->getModelHydrator();
    $data = $hydrator->convertId($data->getId(), $relation->getTargetEntity());
} elseif (...) {
@jdeniau
Copy link
Member

jdeniau commented Jan 27, 2017

Well that's weird because a call to $section->getId () should return the full path, especially after fetching it from your API.

Do your API return /sections/xxxxx or just xxxx?

If it does not return the full URL then I understand the problem and your solution may be the right one.
I will thing about until Monday and make a fix.

Thanks again for your reports!

@jmfeurprier
Copy link
Contributor Author

Our entities do return "xxxxx" as we map the $id properties like this :

    /**
     * @Rest\Id
     * @Rest\Attribute(name="id", type="string")
     */
    private $id;

And here's a sample response from the API:

{
  "@context": "/contexts/Article",
  "@id": "/articles",
  "@type": "hydra:Collection",
  "hydra:member": [
    {
      "@id": "/articles/052113b4-e3b2-11e6-b8b0-0242ac110002",
      "@type": "Article",
      "section": "/sections/d1e8b93e-e3b1-11e6-b8b0-0242ac110002",
      "title": "test",
      "id": "052113b4-e3b2-11e6-b8b0-0242ac110002"
    }
}

We tried mapping the "@id" property from the API to have "/articles/xxxxx" but we had a lot of issues after, maybe we should retry with the recent fixes.

@jdeniau
Copy link
Member

jdeniau commented Jan 29, 2017

OK.

At Mapado we map @id and it's fine for us but your use case is interesting!

I'll try to fix it soon.

@jmfeurprier
Copy link
Contributor Author

jmfeurprier commented Jan 29, 2017

Ok !

Just for information, we have control on the API, which is a Symfony API Platform application (https://github.com/api-platform/api-platform).

On a previous project, we used Symfony + API bundle (preliminary version of API Platform) and we also had the same id/@id output in the API responses.

@jdeniau
Copy link
Member

jdeniau commented Jan 31, 2017

Your point is very interesting.

As a matter of fact, I think the id you use is not the real identifier, which is the iri (we should probably have named it Iri instead of Id everywhere :/ ).

It pointed a thing that I wanted to fix for a long time: the fact the the attribute id may not be the identifier.

Will it be OK for you to have something like this:

/**
 * @Rest\Entity(key="sections")
 */
class Section
{
    /**
     * @Rest\Id
     * @Rest\Attribute(name="@id", type="string")
     */
    private $iri;

    /**
     * @Rest\Attribute(name="id", type="string")
     */
    private $id;
}

This way, you keep your id everywhere, but the sdk will use the @Rest\Id which will be the iri in your case ?
I am working on removing all calls to getId in parallel.

(Just for the record we came from dunglas api-platform too, with only @id (no id), but we moved out for FOS Rest ;) )

@jmfeurprier
Copy link
Contributor Author

Hi, thanks for the feedback.

Just before we go ahead with trying this, can you confirm you suggest adding the annotation "@rest\Id" on both property, or is it a copy-paste mistake?

@jdeniau
Copy link
Member

jdeniau commented Feb 1, 2017

Sorry, copy paste mistake, fixed in the previous comment ;)

It will only work with #47 merged, but you can try it for now by requiring dev-fixRemoveGetId@dev branch.

If you can confirm it's OK.

@jmfeurprier
Copy link
Contributor Author

Excellent ! We will spend some time today to try it, stay tuned.

@jmfeurprier
Copy link
Contributor Author

So far so good !

Retrieval, creation, updates and relations seem to be working as expected now with both IRI and Id fields.

Thanks a lot / gros merci !

@jdeniau
Copy link
Member

jdeniau commented Feb 2, 2017

fixed in v0.18.3

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

No branches or pull requests

2 participants