null relation and NullResource return empty array instead of null

[IlCallo]

In pure Fractal, when you return a null from an include the field is not added on the output array.
If you return a NullResource I think it returns a "fieldname => null" instead (not 100% sure).
In laravel-responder it returns an empty array in both cases, it seems.

I actually recall that yesterday it returned a null field, but I don't remember if it was before or after i started playing with this library 🤔
Can you confirm this or it's a problem with my code? If it's the intended behaviour, would't it be better if it returned just null instead of an empty array?

The use case scenario here is that I have users with an avatar which is optional, so the relation could return null.
If it's not null, it returns the image transformed by its transformer as intended,
If it's null, it returns an empty array.

[flugg]

Strange, this does not sound like expected behavior, I'll look into it.

[flugg]

I can't seem to replicate. Nullable has-one/belongs-to relations output null for me when not set. Do you have some more information I can work?

[IlCallo]

I guess it's problem somewhere in my code then, I'll dig deeper and let you know if I find why this is happening

[IlCallo]

Ok, I came back on the problematic code and found out some more info.
The empty-array-instead-of-null is resulting from the includeMembers I used to overcome the add-pivot-data problem (see #81, I don't know how to share a link to a specific post).

The cause seems to be the fact that I provide an array to $this->resource($data) instead of a Model or Collection, and I think that it's related to the missing support of primitive data (which Fractal 0.17 has).

This have two effects:

• the empty-array-instead-of-null mentioned early;
• the serializer somehow strips out the "data" namespacing and the entity data in the resulting JSON is flattened together with decorators data, below an example of how it's outputted vs how it should be, with dummy data.

How it is with the bug:

[status] => 200
[success] => true
[id] => 3
[name] => ut
[created_at] => [ / data / ]
[is_admin] => 1
[members] => [
    [
        [id] => 28
        [display_name] => Kris Schowalter
        [avatar] => []    <=== wrong empty array
        [pivot] => [
            [admin] => 1
            [created_at] =>[ / data / ]
        ]
   ]
   [
        [id] => 29
        [display_name] => Prof. Branson Senger Sr.
        [avatar] => []    <=== wrong empty array
        [pivot] => [
            [admin] => 0
            [created_at] => [ / data / ]
        ]
   ]
],

How it should be:

[status] => 200
[success] => true
[data] => [    <=== normal namespacing
    [id] => 3
    [name] => ut
    [created_at] => [ / data / ]
    [is_admin] => 1
    [members] => [
        [
            [id] => 28
            [display_name] => Kris Schowalter
            [avatar] => null    <=== null instead of empty array
            [pivot] => [
                [admin] => 1
                [created_at] =>[ / data / ]
            ]
        ]
        [
            [id] => 29
            [display_name] => Prof. Branson Senger Sr.
            [avatar] => null    <=== null instead of empty array
            [pivot] => [
                [admin] => 0
                [created_at] => [ / data / ]
            ]
        ]
    ]
]

[IlCallo]

I pinpointed the problem with the empty-array-instead-of-null problem (which can be related to the second strange behaviour I reported right above).

It's because for transform() helper it's used NullSerializer which serializes null resources (like a null relation loaded on the transformer, in my case) as empty arrays instead of just null.
I think this must be changed to keep consinstency with SuccessSerializer, which returns null values as expected, or remove the field at all (don't remember what does Fractal about null values, I think it removes the field).
Probably this has been done in order to return always an array even if no given data is provided, if this is the case I'm not really sure about how it can be changed maintaining same behaviour, I think it's not possible. Still I prefer to get a null even in that case.
I got that problem because in includeMembers I was transforming data with the helper before returning it wrapped in a $this->resource($data) call.
It's not even possible to specify a custom serializer, so there is no possible workaround, as far as I know, other than redefining the transform helper or copying the code inside the helper definition.

[flugg]

I see. I'm actually not sure why the NullSerializer returns empty arrays for null resources. I believe this is a mistake, I'll look into it.

[IlCallo]

I tryed to modify it to return null and an error bubbled up from fractal package.
I've also checked Fractal serializers and they return [] too, so I guess it's some defaults behaviour inside Fractal that is managed at some point of the serializing process...

IF I remember well about Fractal removing null fields, and I cannot assure about it

[flugg]

I figured it out. NullSerializer has sideloadIncludes() set to true, which attempts to merge the data with sideloaded data - which fails because it's not an array. However, it doesn't actually use sideloads for anything, so setting it to false and returning null from the null() method seems to do the trick!

[IlCallo]

I'm not understanding what "including side-loads" actually means...

I see that the ArraySerializer (which is the base of your SuccessSerializer) keep the default implementation of the method defined on SerializerAbstract, which returns false.
In other words, you can just remove the method from SuccessSerializer and rely on the default one.

In Fractal serializers, only the JsonApiSerializer overrides that method to return true.

[flugg]

The sideload including is an additional step used for adding more data to the final output. For instance, the JsonApiSerializer from Fractal uses it to add relationship information below the actual data. I thought I actually used the sideloadIncludes() method for something for SuccessSerializer. But it seems like the output stays the same even after removing both the sideloadIncludes() and includedData() method (which is only called if sideloadIncludes() is true). I'll do some more checking to make sure it's the same without, but removing this from SuccessSerializer will also fix the NullSerializer issue as you pointed out.

[IlCallo]

Ok, here is what I found about this issue and my hypotesis:

• Fractal translated null to an empty array for "consistency" reasons: they want all branchs of the JSON to be array-based;
• sideloadIncludes and includedData are not used in this library, but given that you expressed many times your idea of staying as much decoupled from Fractal as possible, I may imagine that you didn't want to rely on the underlying Fractal defaults method because they could change, so you just copied all methods, updating something, instead of overriding only the methods which were going to change. You probably saw that those two methods didn't break anything and just left them there (still dunno why sideloadIncludes returned true, maybe it was like this in a previous version of Fractal and you copied it from there?);
• I also noticed that you removed, in NullSerializer, the ability to add meta data (pagination, cursor, etc), which I guess is a consequence of removing the data namespace;
• You removed also the meta namespace for metadata informations, which again I think is a consequence of removing the data namespace from collections (Fractal have it, but you remove it inside mergeIncludes);
• Your paginator and cursor methods are different from Fractal implementation: either you simplified them or copied them from a previous version; on this regards I'd suggest to rely on Fractal implementation where possible and copy the needed methods only when and if the package moves to another underlying transformation manager (Laravel Resources in this case) to avoid missing possible fixes/enhancements.

Given this, I changed null() return value from [] to null and removed sideloadIncludes and includedData from SuccessSerializer, then I updated the test for the NullSerializer and removed the tests for the methods removed from SuccessSerializer.
Here you can check the changes I've made.
I'll now test this setup on my project.
Tell me what do you think about this.

[IlCallo]

An ever better idea would be to return null if the relation refers to a sigle entity and an empty array when the relation refers to a collection of entities, but we currently have no way to know it, right?

[IlCallo]

Ok, the fix proposed won't work alone.
This is because Fractal allows to it's methods to return null (return types are not used) while you used the return type array on:

• make() in FractalTransformFactory and TransformFactory related interface;
• transform() in Transformer class and the interface with the same name;
• transform() in TransformBuilder;
• transform() in the helper file.

This can be resolved removing the return type or, when if using PHP7.1 or higher, using the nullable return type ?array.
This kind of behaviour cannot be checked with Unit tests, that's why you probably though it was ok to just remove those methods and change to null the null() method on NullSerializer.

I think it's better to start writing a test suite with Feature tests, but I'll leave to you to set the format, given that you said you were already going to do that.

return null if the relation refers to a single entity and an empty array when the relation refers to a collection of entities

I noticed that, given that Laravel always returns a Collection for xxx-many relationships but it returns null on missing not-xxx-many relationships, this behaviour already take place with the updates I did.
This need a Feature test too.

[flugg]

You probably saw that those two methods didn't break anything and just left them there (still dunno why sideloadIncludes returned true, maybe it was like this in a previous version of Fractal and you copied it from there?);

I believe I used the sideload includes in a previous version of the serializer, and then found a way to simplify the code for v2, but forgot to remove the methods. I believe these do absolutely nothing right now.

I also noticed that you removed, in NullSerializer, the ability to add meta data (pagination, cursor, etc), which I guess is a consequence of removing the data namespace;

The reason NullSerializer exists is that Fractal doesn't have an API for transforming data without also serializing. This limitation caused me headaches when dealing with web sockets, as I also wanted to expose all data in its transformed state. So the idea was to create a serializer which did absolutely nothing and apply that when using the transform() helper or Transformer service class. Pagination and meta data is part of the serialization part and therefore left out for it.

Your paginator and cursor methods are different from Fractal implementation: either you simplified them or copied them from a previous version;

It should be pretty much the same for SuccessSerializer, with the exception of the keys being camel cased. (It calls the parent method here).

An ever better idea would be to return null if the relation refers to a sigle entity and an empty array when the relation refers to a collection of entities, but we currently have no way to know it, right?

Yes, as you pointed out, this is already the case.

This is because Fractal allows to it's methods to return null (return types are not used) while you used the return type array on:

Indeed, this is something I was looking briefly at when working on supporting primitive resources. I agree this is an unwanted limitation. If we change the contracts I think we should push a new major version, though, as people might rely on these.

I think it's better to start writing a test suite with Feature tests, but I'll leave to you to set the format, given that you said you were already going to do that.

Yeah, I'll get on this soon :)

[IlCallo]

If we change the contracts I think we should push a new major version, though, as people might rely on these.

I'm not sure about this.

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards-compatible manner, and
PATCH version when you make backwards-compatible bug fixes.

This is the definition of versions following the Semantic Versioning.
Allowing nulls in return type "supersets" the current API, so I think it can be considered a backward compatible adjustment, so a minor version 🤔

Anyway, shall I make a PR? Then we can just leave it there until you prepare the tests

[flugg]

This should be fixed with #92

[OzanKurt]

The $this->null() still returns empty array. I couldn't find a solution but passing the relation as a Primitive resource.

[jackkitley]

The $this->null() still returns empty array. I couldn't find a solution but passing the relation as a Primitive resource.

Thanks. i also got this error. Thought it would have been fixed by now.
Your solution fixed.

@flugg is there a better solution for this? thanks

    "class": "TypeError",
    "message": "Flugg\\Responder\\Serializers\\NoopSerializer::null(): Return value must be of type array, null returned",
    "code": 0,
    "file": "/var/www/html/vendor/flugger/laravel-responder/src/Serializers/NoopSerializer.php:51",