Mapped NULL values are serialized outside of the _embedded key

jguittard commented 5 years ago

[x] I was not able to find an open or closed issue matching what I'm seeing.
[x] This is not a question. (Questions should be asked on slack (Signup for Slack here) or our forums.)

Provide a narrative description of what you are trying to accomplish.

Code to reproduce the issue

User.php

namespace App;

class User
{
   /**
    * @var string
    */
   protected $id;

   /**
    * @var string
    */
   protected $name;

   /**
    * @var string
    */
   protected $email;

   /**
    * @var Avatar
    */
   protected $avatar;

   public function getName(): ?string
   {
       return $this->name;
   }

   public function setName(?string $name): User
   {
       $this->name = $name;
       return $this;
   }

   public function getEmail(): ?string
   {
       return $this->email;
   }

   public function setEmail(?string $email): User
   {
       $this->email = $email;
       return $this;
   }

   public function getAvatar(): ?Avatar
   {
       return $this->avatar;
   }

   public function setAvatar(?Avatar $avatar): User
   {
       $this->avatar = $avatar;
       return $this;
   }
}

Avatar.php

namespace App;

class Avatar
{
   /**
    * @var string|null
    */
   protected $id;

   /**
    * @var string|null
    */
   protected $url;

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

   public function setId(?string $id): User
   {
       $this->id = $id;
       return $this;
   }

   public function getUrl(): ?string
   {
       return $this->url;
   }

   public function setUrl(?string $url): User
   {
       $this->url = $url;
       return $this;
   }
}

config.php

MetadataMap::class => [
    [
        '__class__' => RouteBasedResourceMetadata::class,
        'resource_class' => App\User::class,
        'route' => 'api.user',
        'extractor' => ClassMethodsHydrator::class,
    ],
    [
        '__class__' => RouteBasedResourceMetadata::class,
        'resource_class' => App\Avatar::class,
        'route' => 'api.avatar',
        'extractor' => ClassMethodsHydrator::class,
    ],
],

$renderer = new JsonRenderer();
$avatar = new Avatar();
$avatar->setId('1234');
$avatar->setUrl('https://superfastcdn.com/myavatar.png');
$user = new User();
$user->setId('5678');
$user->setName('John Doe');
$user->setEmail('john@doe.com');
$user->setAvatar($avatar);

Expected results

$resource = $resourceGenerator->fromObject($user, $request);
echo $renderer->render($resource);

$user->setAvatar(null);

$resource = $resourceGenerator->fromObject($user, $request);
echo $renderer->render($resource);

{
    "name": "John Doe",
    "email": "john@doe.com",
    "_embedded": {
        "avatar": {
            "id": "1234",
            "url": "https://superfastcdn.com/myavatar.png",
            "_links": {
                "self": {
                    "href": "https://api.acme.com/avatars/1234"
                }
            }
        }
    },
    "_links": {
        "self": {
            "href": "https://api.acme.com/users/5678"
        }
    }
}

{
    "name": "John Doe",
    "email": "john@doe.com",
    "_embedded": {
        "avatar": null
    },
    "_links": {
        "self": {
            "href": "https://api.acme.com/users/5678"
        }
    }
}

Actual results

{
    "name": "John Doe",
    "email": "john@doe.com",
    "_embedded": {
        "avatar": {
            "id": "1234",
            "url": "https://superfastcdn.com/myavatar.png",
            "_links": {
                "self": {
                    "href": "https://api.acme.com/avatars/1234"
                }
            }
        }
    },
    "_links": {
        "self": {
            "href": "https://api.acme.com/users/5678"
        }
    }
}

{
    "name": "John Doe",
    "email": "john@doe.com",
    "avatar": null,
    "_links": {
        "self": {
            "href": "https://api.acme.com/users/5678"
        }
    }
}

weierophinney commented 5 years ago

Since you have the scenario mapped here, could you please submit a pull request demonstrating the issue? I can work on a solution this week if so.

jguittard commented 5 years ago

@weierophinney demonstrating the issue = failing unit test ?

weierophinney commented 5 years ago

Indeed!

On Mon, Jan 7, 2019, 5:47 PM Julien Guittard <notifications@github.com wrote:

@weierophinney https://github.com/weierophinney demonstrating the issue = failing unit test ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-expressive-hal/issues/52#issuecomment-452123498, or mute the thread https://github.com/notifications/unsubscribe-auth/AABlVyIfRiJ4156tgWfJ0Gmg66A6L5qkks5vA9yigaJpZM4Zy6QM .

jguittard commented 5 years ago

@weierophinney see #53

weierophinney commented 5 years ago

After seeing #53, I took a closer look at the expected/actual results you had above, and I'm not 100% convinced that what you expect is correct.

In the specification description for _embedded, it notes:

The reserved "_embedded" property is OPTIONAL

It is an object whose property names are link relation types (as defined by [RFC5988]) and values are either a Resource Object or an array of Resource Objects.

Embedded Resources MAY be a full, partial, or inconsistent version of the representation served from the target URI.

The specification is explicitly stating that the values for each relation must be an object or array of objects (what we call collections in this package).

As such, because you have assigned a null value, it cannot be considered an embedded resource, which is why the current code pushes it back into the parent resource.

One way I've looked at HAL: the complete resource is the combination of its properties, and the various _embedded properties, where the latter indicate related resources. A null value indicates there is no related resource, and thus would not be in the _embedded section. I think pushing it into the _embedded relations would be a violation of the specification, no?

jguittard commented 5 years ago

Two things before going deeper into business considerations:

In my above example, avatar is a resource whose value is in the latter case set to null. The fact that it's null should not push it back to a regular property.
How about simply set an empty array of properties upon serialization of the embedded null object?

weierophinney commented 5 years ago

The point is that, for purposes of HAL, a resource MUST be an object, and, specifically, an object with relational links. null does not fall under that category.

Neither does an empty object - UNLESS you provide relational links for it (e.g., a POST URL to which one can make a request to create a resource of that type). Even then, I'd argue the relational link goes in the parent:

{
  "_links": {
    "avatar": { "href": "/users" }
    "self": { "href": "..." }
  },
}

jguittard commented 5 years ago

How does HAL then represent the property of an object whose value is not set? It just not represents it? When the client of the API parses the JSON object, following my exemple, it expects the avatar property to be read into the _embedded key. Following your POV, it should first parses this entry, then if it doesn’t find the key, it should try to match it in the parent entry? Not very consistent, IMHO...

weierophinney commented 5 years ago

I think the problem is assuming that a value is either under the top-level or an _embedded key. The idea on the client-side should be to create an object that merges the values from both:

.then(function(payload) {
  let user = Object.assign(payload, payload._embedded);
  delete user._embedded;
  return user;
})

If you do pre-processing of the returned payload such as the above, you will not have to worry about where in the representation a property lies; your code will just see a unified "user" object, where some of the properties just happen to be objects that also contain links.

weierophinney commented 4 years ago

This repository has been closed and moved to mezzio/mezzio-hal; a new issue has been opened at https://github.com/mezzio/mezzio-hal/issues/4.

zendframework / zend-expressive-hal