cloudcreativity/laravel-json-api

[5.x] Missing meta

colq2 opened this issue · 8 comments

colq2 commented

Hey @lindyhopchris, thanks for the v5! Referencing #632 here, we found an issuer during the Upgrade.

Unfortunately, we are missing the meta of resources. Already diving into the code, I discovered that in the neomerx package the methods were renamed from getPrimaryMeta and getInclusionMeta to getResourceMeta.

We could determine this because the function getPrimaryMeta and getInclusionMeta are not called.

Diving into the code, we could make it work by editing CloudCreativity\LaravelJsonApi\Schema

    public function getResourceMeta($resource)
    {
        if(method_exists($this->provider, 'getPrimaryMeta')){
            return $this->provider->getPrimaryMeta($resource);
        }

        if(method_exists($this->provider, 'getInclusionMeta')){
            return $this->provider->getInclusionMeta($resource);
        }

        return null;
    }

I would like to create a PR, but I don't know how to solve this properly.

One open question: Should we callgetPrimaryMeta and getInclusionMeta?
Second: How to test this?

The SchemaInterface also defines getIdentifierMeta. If I got it right the getIdentifierMeta is for the meta which is on the same level as type and id but not in a resource in the included data. But getResourceMeta is the opposite.

On the other side, getPrimaryMeta was present in the primary requested resource and getInclusionMeta was present in the included data. Am I right here?

Do you maybe know what could cause this issue?

Hi! Thanks for raising this. I'll have a look at this as a priority next time I'm working on Open Source things. Unfortunately this week I'm quite busy, so it's likely to be at the weekend.

colq2 commented

No worries. Thank you for maintaining this!

colq2 commented

Hey @lindyhopchris have you by any chance already had the time to look into this? Please let me know if I can help in any way 👋

Sorry, no I haven't had time to look at this.

I'm not sure off the top of my head how to solve this one, because for backwards-compatibility we need to distinguish between when the resource is being serialized in the data member or in the included member. I need to dig into the code to find out how we determine that, plus whether it's possible to determine that within the new getResourceMeta.

Sorry, this one isn't straight forward which is why I need to find a chunk of time to dig into it.

colq2 commented

I can totally understand that! Just let me know if I can do anything else.

@colq2 so I've looked into this.

There is no way when generating meta for a resource to determine whether the resource is in the data or included member of the JSON:API document. This means there is no backwards compatible way to support that distinction between meta on a resource.

Neomerx must have dropped support for it. Which I actually agree with - it never made any sense to me as to why the meta should vary dependning on that distinction, as meta about the resource should not be dependent on location within the document.

So there's no way for me to support how you've previously been doing it.

Seeing as there's no distinction, I'd suggest detecting the presence of a single method, getResourceMeta(object $resource) on the schema provided that Schema delegates to. I can't see any other way forward, but thought I'd check with you in case you had any thoughts?

I consider the lack of support for resource meta a bug, so this definitely does need fixing.

colq2 commented

@lindyhopchris thank you for looking into this!

I also agree in dropping it. Looked into our resources and we almost used everywhere.

public function getInclusionMeta($resource)
{
    return $this->getPrimaryMeta($resource);
}

I don't fully understand your suggestion. Did you see my suggestion in the first comment?

Is there anything against checking if the old methods?

Maybe it is a good idea to fully drop the old methods and put them into the upgrade guide.