What's happened to include relationships in schema `getRelationships` method

lindyhopchris commented 5 years ago

Quick question on v3... what's happened to the $includeRelationships argument that used to be passed into the getRelationships method on the schema?

We use this in every schema to do something like the following:

    public function getRelationships($resource, $isPrimary, array $includeRelationships)
    {
        return [
            'comments' => [
                self::SHOW_SELF => true,
                self::SHOW_RELATED => true,
                self::SHOW_DATA => isset($includeRelationships['comments']),
                self::DATA => function () use ($resource) {
                    return $resource->comments;
                },
            ],
        ];
    }

How do we do this in v3?

Note that we do this to follow the spec. For a relationship data member the spec says it is resource linkage. The definition of that is:

Resource linkage in a compound document allows a client to link together all of the included resource objects without having to GET any URLs via links.

I.e. we only need to include the relationship data if the related resource is going to appear in a compound document.

If it is not possible to do the above in v3 then it would not be possible for us to upgrade, as removing this behaviour would break clients connecting to our API, plus affect several client libraries that we use on the client-side ourselves.

neomerx commented 5 years ago

Short Answer

It's not needed.

Long Answer

$includeRelationships in v1 existed because it didn't have Identifiers which were added in v3. So you could use mostly empty resources without attributes and relationships. Kinda hack.

At that time it was OKeish solution but if you think the existence of $includeRelationships is a broken logic. What does the following line?

self::SHOW_DATA => isset($includeRelationships['comments'])

It makes a decision if data should be shown (added to included section). It does not really, but interprets desiction made by someone else before encoding. Without a usage context, it looks acceptable but how it looks as a part of an application?

server receives request with some paths to include
server validates inputs, constructs SQL requests and loads resources (decistion to load/not load is made here)
server encodes data and the encoder makes decistion about inclustion of relationships... again...

It just should not be there.

OK, how to deal with it?

Decide if requested include paths are allowed
Load the data and prepare resources
Encode

Within a Schema if $resource had data then add DATA section (it's called RELATIONSHIP_DATA now) and omit it if there is no data.

I don't have to deal with lazy loading so I can't tell how easy to manage what they should and should not load and if you want to look at a place where Schema::getRelationships is used, it's here. But please Give me the $currentPath will not be your first idea :smile: Because why a resource should return different relationships depending on the order parser looks at it?

neomerx commented 5 years ago

A solution could be as simple as translate requested paths (for example, post's comments,comments.author) to a simple map which relationships should be loaded.

$lazyLoadSchemaHelper = [
    'posts'    => ['comments' => true],
    'comments' => ['author' => true],
];

// with usage in a Schema as
$shouldLoadRelationship = isset($lazyLoadSchemaHelper[$schema->getType()][$relationshipName]);

lindyhopchris commented 5 years ago

There's two different issues with your response...

Issue 1

TLDR: your encoder can no longer encode responses that conform with JSON API's design, and this will break client-side JSON API implementations.

The spec defines the data member of a relationship with two words here: resource linkage.

The definition of resource linkage is:

Resource linkage in a compound document allows a client to link together all of the included resource objects without having to GET any URLs via links.

This means the data member of a relationship only needs to be included as part of a compound document, i.e. if the related resource will appear in the included member of the top-level document. You should be aware that this is exactly what a number of client-side JSON API compliant libraries expect - i.e. they expect that if there is a data member, they can find the related object in the included member of the document. In other words, your Identifier solution will break those client libraries because providing a resource identifier without including the related resource means the compound document does not have full resource linkage - which is a MUST in the spec.

To know whether to include the data member, you need to know at the point a resource is being serialized whether the relationship is included in the a compound document or not. Your $includeRelationships in v1/2 was a simple solution for this. You need to know in the getRelationships method because you do not know where in the document you are encoding - all you know is the specific resource you are encoding.

Now you say that this can be detected from whether the resource has eager loaded the relationship. However this is based on three massive assumptions:

That the resource is in a database. We have a number of non-database resources in multiple production applications.
That when it is in the database, your ORM supports eager loading and it allows you to detect whether the eager loading has occurred. For example, $post->comments might give you the eager loaded comments OR lazy load the relationship - you might not know which.
That you will only over eager load what is in the include path, rather than also having to sometimes eager load related resources for other application concerns. This is common for a number of our resources (again in multiple production applications), which means we cannot detect whether to include the data member of the relationship based on eager loading. We need to know the encoder's intent (i.e. whether the related resource will be in the included member of the document) which can be completely different from whether the server has decided to eager load a related resource. E.g. in Laravel, it is possible to set default eager load paths that are always loaded. However this does not mean we want to put the identifier in the encoded data - we follow the spec to give the client exactly what it asked for. This helps to keep response sizes low - i.e. don't add in information that the client has not asked for, even if it is eager loaded.

So in summary, your statement that "It is not needed" might be true for your implementation, but is absolutely, 100%, not true for all implementations. The decision on relationship data is not exclusively taken before the encoding, so the schema needs that information. It needs to know: will this related resource be in the included member of the document?, which is not the same as: has the server eager loaded this relationship?. You need to re-introduce $includeRelationships, which I'm guessing is going to have to be a v4 release.

Issue 2

Removing a feature might work fine for your implementation. However if the feature has been used by other people (which you have to assume as you've open-sourced this package), then you have made a breaking change to their API if they are not able to generate the same encoded response as they were able to do before. I.e. their API will not be the same, which breaks any client that is connecting to it.

Now you might say, well just update the connecting clients. However we have used this package for APIs that third-party companies connect to. Those companies have built their own applications using our API. We cannot go to them and say that our API will now send completely different responses from what is documented, and that they have to re-write their applications because you removed a feature from your package.

I.e. while your PHP code might change from v2.0 to v3.0 and that might force us as PHP programmers to make changes internally, externally the API needs to stay the same. Your decision to remove a feature that you no longer want to use has consequences for the rest of us.

lindyhopchris commented 5 years ago

Because why a resource should return different relationships depending on the order parser looks at it?

Because the whole point of the spec is to give the client control of what it is in the document, not the server!

lindyhopchris commented 5 years ago

As an example, the Ruby JSON API serializer only serializes the data member of a relationship if it is in the include specified by the client: https://github.com/fotinakis/jsonapi-serializers#compound-documents-and-includes

Here's a good write-up from the Ember community:

In 0.3.x, Mirage's JSONAPISerializer included all related foreign keys whenever serializing a model or collection, even if those relationships were not being included in the payload.

This actually goes against JSON:API's design. Foreign keys in the payload are known as Resource Linkage and are intended to be used by API clients to link together all resources in a JSON:API compound document. In fact, most server-side JSON:API libraries do not automatically serialize all related foreign keys, and only return linkage data for related resources when they are being included in the current document.

https://www.ember-cli-mirage.com/docs/upgrading#0-3-x-0-4-upgrade-guide

neomerx commented 5 years ago

I'm sorry for stating very obvious things but steps to resolve an issue might look like 1) A problem is defined. 2) A solution plan is defined. 3) The solution is implemented and tested. 4) The solution is validated.

We are currently at 0)

A short and clear definition for 1) is needed.

I'd like to exclude some of your claims from the discussion.

Issue 1 TLDR: your encoder can no longer encode responses that conform with JSON API's design, and this will break client-side JSON API implementations.

There are actually two mentioned

a) encoder can no longer encode responses that conform with JSON API's design

All the encoding tests were migrated to v3 with all imaginable combinations of includes, fieldsets, and etc. And even a couple of minor inconsistencies were fixed.

b) this will break client-side JSON API implementations

It is a change that breaks backward compatibility.

Issue 2

... not able to generate the same encoded response ...

... to remove a feature that you no longer want to use ...

I have to go atm. I'm asking you to think and understand that this is an encoder, not ORM and Schema implementation is outside of the project scope. BaseSchema is a basic implementation which you even don't have to use. The encoder does not decide which paths to include. It's told to do which ones.

lindyhopchris commented 5 years ago

Thanks for the response. I'm not trying to be awkward, it's just your v3 encoder does not allow me to create the same encoded content that I'm able to do on v1/v2. This indicates there's tests missing.

I can help define what the issue is. How about I add a test to the 2.x branch for the scenario? Then you can back port the test into v3?

neomerx commented 5 years ago

encoder does not allow me to create the same encoded content

I believe it's rather encoder does allow me to create the same encoded content but not the way it used to be

How about I add a test to the 2.x branch for the scenario? Then you can back port the test into v3?

That's likely to be pretty ordinary data with includes and fields set filters where information about the include paths and the filters a Schema would take from input arguments of getRelationships method.

For both v2 and v3 information about the paths and filters are inputs. In the previous version, encoder took those inputs and then passed it through all the internal methods down to Schemas which are also inputs.

When you've got a Request for paths you parse them and load data from a database accordingly then you pass the data to encoder. Your ORM cannot do that for various technical reasons? OK, pass it to your Schemas.

Mind you, Schemas are totally developers' responsibility and the encoder works with developers' implementation of interface SchemaInterface and sends only input data or data received from the Schemas.

If you have paths a,a.b,a.b.c and/or filters 'type1' => ['attribute1', 'attribute2', 'relationship1', ...] you've everything you need to load data properly or create your Schemas before passing your inputs to encoder. Please note filters are already exactly what you want to have in Schemas. Would it be nice to have them back from the encoder? Filters? Probably not because encoder adds nothing to it. Paths? Probably. The encoder parses it anyway so it might be helpful.

I would say the problem is that the previous version parsed your input paths/filters and shared it back to your code in Schemas which was convenient. The current version does not.

If 1) is ready and we are on the same page we can go to 2).

lindyhopchris commented 5 years ago

I suppose we're almost on the same page, but not quite. FWIW I'm really not trying to be awkward here! This is a feature that led us to choose your package, so much more than a convenience!

Creating a map to pass to the schema isn't as simple as you say it is. Given this request:

GET /api/posts?include=comments.user.country,author.top-posts

What happens if the author relationship actually returns a users resource? How do I know to create this mapping?:

$lazyLoadSchemaHelper = [
    'posts'    => ['comments' => true, 'author' => true],
    'comments' => ['user' => true],
    'users' => ['country' => true, 'top-posts' => true],
];

I'd need to know the inverse resource type for each relationship at the point I'm working out the above mapping. With v1/v2 I quite rightly don't need to know this, because the encoder will encounter a User object when it gets the data for the author relationship, and then knows to pass this to the UserSchema. That's more than a convenience; the encoder is the thing iterating through the stack, so is best placed to work this out during the encoding.

One other thing to say about this example is that the client is asking for:

the country to be included for any user of a comment.
the top-posts to be included for the post's author.
the country and top-posts to be included for the post's author if that author is also the user of a comment.

So the mapping of 'users' => ['country' => true, 'top-posts' => true] does not work because it implies I need to include country and top-posts for every user. Plus if top-posts returns a post resource, the mapping of 'posts' => ['comments' => true, 'author' => true] implies I need to include the comments and author for all of those as well!

Paths? Probably. The encoder parses it anyway so it might be helpful.

I'd say it's always helpful because only the encoder knows where it is (the current path) when it has to encode a resource. The above map, even if I can compile it, does not actually tell me where (the path) a resource is being serialized by a schema. The only thing that can tell a schema this is the encoder, which is why in v2 you needed the stack to work out the include paths to give to the https://github.com/neomerx/json-api/blob/v2.x/src/Encoder/Parser/ParserManager.php#L73-L80

neomerx commented 5 years ago

I've created a temporary branch for the issue. Please have a look at Tests\Extensions\Issue236 where I demonstrated how to have a current path of the resource being parsed in a Schema. I think it should make the issue solution pretty straightforward.

lindyhopchris commented 5 years ago

I have only had a brief look at this. I get the idea of changing the internals via the factory, but the test is not equivalent to the functionality that is lost from v1/v2. So setting up the test to create the two scenarios would be an improvement.

In the example that you've set up, in the comments schema, you need to know that the author is going to be included so that you know whether or not to have data for the relationship.

The issue test needs two scenarios to replicate what was possible with v1/v2:

Scenario 1: No include paths

    public function testDataSerializationWithoutIncludePaths(): void
    {
        $actual = CustomEncoder::instance($this->prepareSchemas())
            ->withUrlPrefix('http://example.com')
            ->encodeData($this->prepareDataToEncode());

        $expected = <<<EOL
        {
            "data" : {
                "type" : "comments",
                "id"   : "5",
                "attributes" : {
                    "body" : "First!"
                },
                "relationships" : {
                    "author" : {
                        "links" : {
                            "self": "http://example.com/comments/5/relationships/author",
                            "related": "http://example.com/comments/5/author"
                        }
                    }
                },
                "links":{
                    "self" : "http://example.com/comments/5"
                }
            }
        }
EOL;
        self::assertJsonStringEqualsJsonString($expected, $actual);
    }

Scenario 2: With Include Paths

    public function testDataSerializationWithIncludePaths(): void
    {
        $actual = CustomEncoder::instance($this->prepareSchemas())
            ->withUrlPrefix('http://example.com')
            ->withIncludedPaths([Comment::LINK_AUTHOR])
            ->encodeData($this->prepareDataToEncode());

        $expected = <<<EOL
        {
            "data" : {
                "type" : "comments",
                "id"   : "5",
                "attributes" : {
                    "body" : "First!"
                },
                "relationships" : {
                    "author" : {
                        "data": {
                            "type": "people",
                            "id": "42"
                        },
                        "links" : {
                            "self": "http://example.com/comments/5/relationships/author",
                            "related": "http://example.com/comments/5/author"
                        }
                    }
                },
                "links":{
                    "self" : "http://example.com/comments/5"
                }
            },
            "included" : [{
                "type" : "people",
                "id"   : "42",
                "attributes" : {
                    "first_name" : "Peter",
                    "last_name"  : "Weller"
                },
                "relationships":{
                    "comments" : {
                        "links": {
                            "self": "http://example.com/people/42/relationships/comments"
                        }
                    }
                },
                "links": {
                    "self": "http://example.com/people/42"
                }
            }]
        }
EOL;
        self::assertJsonStringEqualsJsonString($expected, $actual);
    }

neomerx commented 5 years ago

With a few additions in tests, you can have both current path and the all path to include. Having both, calculating the next paths should be trivial.

Please have a look.

lindyhopchris commented 5 years ago

Ok I see where you're going with this. It feels like a significant step backwards but I can see how I can extend the implementation to do what it did before.

Considering there's a PositionInterface object, I'd much rather that's passed through as that is essentially what's going on here - the schema is being told what the encoder's position is.

I'd also need the $hasData decision to be delegated to a protected method on the parser.

I've pushed a branch with both of the above changes for you to consider.

neomerx commented 5 years ago

That's my turn to rant :smiley: What I've learned is that the more the lib provides access to implementation specifics the more insults and downvotes I get in case of changes. So I deliberately choose only the curent path instead of PositionInterface.

I'd also need the $hasData decision to be delegated to a protected method on the parser.

I don't understand why it's need but not want. Why you can't just avoid fill it in or unset() in the first place?

lindyhopchris commented 5 years ago

My preference would be to have a value object rather than a string. This whole issue is around the schema needing to know the current position of the resource it's serializing, so as there's a value object that represents that our normal approach would be to pass it through. The getPosition() method on the parser allows me to do that anyway.

Re: $hasData:

I don't understand why it's need but not want. Why you can't just avoid fill it in or unset() in the first place?

We use the SHOW_DATA constant in earlier versions in 100% of our relationships. In applications that have 100s of relationships. This is a horrific developer experience:

    public function getRelationships($resource): iterable
    {
        \assert($resource instanceof Comment);

        /** @var PositionInterface $position */
        $position = \func_get_arg(1);

        $rel = [
            Comment::LINK_AUTHOR => [
                self::RELATIONSHIP_LINKS_RELATED => false,
                self::RELATIONSHIP_META          => [
                    'current_path'      => $position->getPath(),
                    'all_include_paths' => $this->allIncludePaths
                ],
            ]
            // lots of other relationships...
        ];

        if (\in_array('author', $this->allIncludePaths)) {
            $rel[Comment::LINK_AUTHOR][self::RELATIONSHIP_DATA] = $resource->{Comment::LINK_AUTHOR}; 
        }

        // lots of other if statements for all the other relationships...
        // ...multiplied by all the relationships in all the schemas in huge production APIs.

        return $rel;
    }

The code is instantly a lot less clear, you can't see a relationship as a single array, and in large teams in commercial settings clean code means easier to maintain. That's far more important than whatever tiny performance improvement has been gleaned by removing the SHOW_DATA constant.

Plus in a commercial context I need to justify to bosses/customers why I'm spending so long going through converting 100s of relationships!!!

Can you re-introduce the SHOW_DATA constant? You want to remove it, by why do you need to remove something that people were using? It's not just me using it... pretty much everyone using our Laravel JSON API package is using it - which is a lot of developers, who haven't been hit by this change yet because we've not upgraded to v3 of your package yet.

neomerx commented 5 years ago

SHOW_DATA is not some magic. It was and cannot be anything more than

if ($relationship[self::SHOW_DATA] === false) {
    unset($relationship[self::DATA]);
}

In v3 you can do either same unset or better do not fill it in in the first place.

Anyway, I think what does make sense is writing a decent getRelationships that can be reused. I expect it will need neither position nor SHOW_DATA.

Do you parse include paths to use it against input validator or database? Or just send them straight to the encoder? If so, what is the parsed format?

neomerx commented 5 years ago

@lindyhopchris ping

lindyhopchris commented 5 years ago

It's an Easter holiday here in the UK.

Of course I understand that SHOW_DATA is just an unset - but it's magic is that it happens during the iteration while processing the array that represents the relationships.

Surely we're talking about one line here:

$hasData = $relationship[self::RELATIONSHIP_SHOW_DATA] ?? \array_key_exists(self::RELATIONSHIP_DATA, $relationship);

neomerx commented 5 years ago

Oh, sorry :smile:

Let me know when you're back so we can finish reusable getRelationships. I'd like to know how you work with input include paths.

Do you parse include paths to use it against input validator or database? Or just send them straight to the encoder? If so, what is the parsed format?

lindyhopchris commented 5 years ago

Do you parse include paths to use it against input validator or database? Or just send them straight to the encoder? If so, what is the parsed format?

We validate the allowed JSON API include paths, i.e. do not convert them for validation.

We then convert them to eager loading paths and ask the query to eager load those paths. The format is a dot notation for eager loading. This is if the resource supports eager loading - which it may not.

However as I've stated before, these may not be the only paths that are eager loaded: as the developer may choose to eager load other paths based on other application needs. As we cannot tell why a path may be eager loaded in the schema (i.e. the intent), this is why we have always be reliant on the previously provided $includeRelations. I.e. we use that even if eager loading has occurred - so we are using it on 100% of our schemas, both those that support eager loading and those that don't.

neomerx commented 5 years ago

I've made a reusable class that takes requested include paths and fieldsets from the input request and returns expected include paths and fields for a given current path and type. Kinda _goodold parser intentions.


class SchemaPathsHelper
{
    private const PATH_SEPARATOR = '.'; //  = DocumentInterface::PATH_SEPARATOR;

    private const FIELD_SEPARATOR = ',';

    /** @var array */
    private $fastRelationships;

    /** @var array */
    private $fastRelationshipLists;

    /** @var array */
    private $fastFields;

    /** @var array */
    private $fastFieldLists;

    public function __construct(iterable $paths, iterable $fieldSets)
    {
        foreach ($paths as $path) {
            $separatorPos = \strrpos($path, static::PATH_SEPARATOR);
            if ($separatorPos === false) {
                $curPath      = '';
                $relationship = $path;
            } else {
                $curPath      = \substr($path, 0, $separatorPos);
                $relationship = \substr($path, $separatorPos + 1);
            }
            $this->fastRelationships[$curPath][$relationship] = true;
            $this->fastRelationshipLists[$curPath][]          = $relationship;
        }

        foreach ($fieldSets as $type => $fieldList) {
            foreach (\explode(static::FIELD_SEPARATOR, $fieldList) as $field) {
                $this->fastFields[$type][$field] = true;
                $this->fastFieldLists[$type][]   = $field;
            }
        }
    }

    public function isRelationshipRequested(string $currentPath, string $relationship): bool
    {
        return isset($this->fastRelationships[$currentPath][$relationship]);
    }

    public function getRequestedRelationships(string $currentPath): array
    {
        return $this->fastRelationshipLists[$currentPath] ?? [];
    }

    public function isFieldRequested(string $type, string $field): bool
    {
        return \array_key_exists($type, $this->fastFields) === false ? true : isset($this->fastFields[$type][$field]);
    }

    public function getRequestedFields(string $type): ?array
    {
        return $this->fastFieldLists[$type] ?? null;
    }
}

$paths = [
    'a1',
    'a2',
    'a1.b1',
    'a2.b2.c2',
];

$fieldSets = [
    'articles' => 'title,body,a1',
    'people'   => 'name',
];

$helper = new SchemaPathsHelper($paths, $fieldSets);

// `Good old` style
$expectedPaths  = $helper->getRequestedRelationships('');
//array (
//  0 => 'a1',
//  1 => 'a2',
//)

$expectedPaths  = $helper->getRequestedRelationships('a2.b2');
//array (
//  0 => 'c2',
//)

$expectedPaths  = $helper->getRequestedRelationships('a2.b2.c2');
//array (
//)

$expectedFields = $helper->getRequestedFields('articles');
//array (
//  0 => 'title',
//  1 => 'body',
//  2 => 'a1',
//)

$expectedFields = $helper->getRequestedFields('non-filtered-type');
//null

// You can also ask if a field is expected to be in output

$shallOutputAttribute    = $helper->isFieldRequested('articles', 'title');
//true

$shallOutputRelationship = $helper->isFieldRequested('articles', 'a1') && $helper->isRelationshipRequested('', 'a1');
// true

lindyhopchris commented 5 years ago

Ok great, so I'm assuming you'll add that to this package?

Also, can you now show me how to use it in a schema without SHOW_DATA being a constant? Assume that a schema might have up to 10 relationships and there might be 50+ schemas?

neomerx commented 5 years ago

I've updated the test which shows how to get information about attributes and relationships to be included in the output.

An equivalent of SHOW_DATA === false would be not returning RELATIONSHIP_DATA.

lindyhopchris commented 5 years ago

Great, but can you confirm that the SchemaFields is being added to the package? There's little point having something that the package needs to support that is hidden in a test folder.

An equivalent of SHOW_DATA === false would be not returning RELATIONSHIP_DATA.

That's one major part of my issue though - what is currently succinct, easily readable and maintainable code, gets changed into a horrific mess of if statements.

I don't understand why we can't put a RELATIONSHIP_SHOW_DATA constant back in and add one line of code when processing the relationship array:

$hasData = $relationship[self::RELATIONSHIP_SHOW_DATA] ?? \array_key_exists(self::RELATIONSHIP_DATA, $relationship);

If you are totally against adding that, can you please move the calculation of $hasData to a protected method so we can extend and override it?

I'm asking for this because it is a need for us, not a would like to have. You may not need it, but it was supported in v2 and I can see no reason for removing something that people are using. In our open source projects we regularly support capabilities that we are not using, but we know are being used for good reasons by the open source community.

neomerx commented 5 years ago

It's nice to hear we've finally come to a mutual understanding :smile:

SchemaFields has excessive attributes and methods so you're likely to tailor it to your needs and half the size to about 70 lines. It was included in the sample solution to demonstrate that having a current path of the resource being parsed in a Schema is sufficient. You'd probably include it into a custom base schema with show data key to make reusable in your schemas.

I totally feel and understand your desire to include a tailored solution for your situation (everybody's doing it). However, balancing it is a common best interest.

neomerx commented 5 years ago

Merged to develop. If everything is OK it will be merged to master and released.

lindyhopchris commented 5 years ago

As it stands we won't be able to use v3.

Can you move this line to a protected method so it can be overridden? https://github.com/neomerx/json-api/blob/master/src/Parser/RelationshipData/ParseRelationshipDataTrait.php#L58

neomerx commented 5 years ago

Can you please explain why it's need but not want?

neomerx commented 5 years ago

There are millions of ways to develop the code and one of the solutions could be a conversion function c() that transforms old style description to new ones. It might be your custom BaseSchema with

class YourBaseSchema extends BaseSchema
{
// const declarations with old namings
const HAS_DATA = ...
...

protected function c(array $oldRelationshipDescription): array
{
    // has_data support
    if (($oldRelationshipDescription[static::HAS_DATA] ?? false) === false) {
        unset($oldRelationshipDescription[static::DATA] );
    }

    // properties mapping
    ...
}

}

lindyhopchris commented 5 years ago

Can you please explain why it's need but not want?

I have, multiple times. But here we go again.

I think we just have a totally different approach to open source... we'd intentionally put things in protected methods so that the developer can easily override for their own implementation. We don't find that this is difficult to maintain.

To use your example, we'd have to do this:

public function getRelationships($resource): array
{
   $path = func_get_arg(1); // this is truly horrific and a classic sign of a badly designed interface.

  return [
    'author' => $this->c([
       self::HAS_DATA => ...,
       self::RELATIONSHIP_DATA => ...
    ]),
    'comments' => $this->c([
       self::HAS_DATA => ...,
       self::RELATIONSHIP_DATA => ...
    ]),
  ];
}

(The other solution would be to loop through the relationships returned, but that extra iteration is substantially more costly than doing it when the relationship array is processed by the parser.)

You'd be asking me to go through every schema, change it from extending your schema, add the func_get_arg call, and then add that c() wrapping to every relationship. You'd then be asking me to tell the 1000s of developers who use the laravel-json-api package to do that to every schema they've written plus all the relationships. All to do what is a standard JSON API server-side practice: only show relationship data if the client has explicitly asked for it.

All this change because that constant has been removed, for the only reason that you aren't using it. If this isn't the definition of a need, I don't know what is. And here's why...

The reality is large scale commercial applications we need to make a commercial (cost) decision as to whether that's an effective use of time. It is not, so we'll need to stay on v2.

In the future we'll need to make a decision on whether we do that upgrade work or whether it's better to write our own encoder. Because this issue has demonstrated that the risk of sticking to your encoder is that we hit these stumbling blocks of you removing something you're not using, but we are 100% reliant on. I acknowledge that's a risk with using any open source project, but this problem is accentuated in this package because of the amount of private methods that we cannot override to customise the implementation.

You can close this issue as the current solution to this issue is to stay on v2.

neomerx commented 5 years ago

I think we just have a totally different approach to open source... we'd intentionally put things in protected methods so that the developer can easily override for their own implementation.

Excuse me, but the whole lib is made on interfaces and highly modular. Do you rebuke me for not having every single method as a part of an interface? The root of the issue is that you ignore user inputs (e.g. filters and include paths) while preparing your data to encode. You've got your data not ready and when the encoder asks your schemas for data to encode it response with 'I do not know' and you blame the encoder (and me personally).

I at least spend time and effort to understand your problem and added a solution so you can deal with your data problem. And your response?

this is truly horrific and a classic sign of a badly designed interface.

Really?

I'm still OK to help you but would appreciate if you behave in a more reasonable and civil manner.

neomerx commented 5 years ago

Also, feel free to completely replace \Neomerx\JsonApi\Parser\IdentifierAndResource::getRelationships (it's only a handful of lines), use ParseRelationshipDataTrait as private/protected or even public and add getNonHorrificRelationships with such extra params as current path and all the bells and whistles you want. I've made quite a big sample for you so for a skillful developer as you it will not take more than half an hour.

neomerx commented 5 years ago

Your IdentifierAndResource would receive in constructor

curent position PositionInterface $position,
FactoryInterface $factory,
resource schema SchemaContainerInterface $container,
resource itself $data

Your getRelationships should yield for every relationship

yield $name => $this->factory->createRelationship(
                $nextPosition,
                $hasData,
                $relationshipData,
                $hasLinks,
                $links,
                $hasMeta,
                $meta
            );

So you can support any format for describing your relationships in your schemas and use Schema::getNonHorrificRelationships instead of that terribly designed Schema::getRelationships.

lindyhopchris commented 5 years ago

The root of the issue is that you ignore user inputs (e.g. filters and include paths) while preparing your data to encode.

This is not the root of the issue, which is why we're struggling to come to an agreement.

OK personally I do not like the idea of copying & pasting so many (87) lines of code to change just a couple of lines of it, but I understand your position.

If we do move onto v3 I'll submit a PR to update the Issue236 tests to reflect how we're actually using it, because how those tests are structured at the moment do not reflect the solution to the problem.

neomerx commented 5 years ago

@lindyhopchris Please have a look at a branch v4-rc. Essentially that's v3 where a Schema has an additional parameter ContextInterface in getRelationships method

    public function getRelationships($resource, ContextInterface $context): iterable
    {
        // ...
    }

I would appreciate your feedback before actually releasing it as the next version.

lindyhopchris commented 5 years ago

The context interface looks good for providing the schema with the info it needs about where in the encoding process it is being uses. So think that's a positive step.

This context would also be required for the getAttributes() method too (because that might want to use the field sets on the context). See #195

So if your preference is to provide the context to specific methods, it needs to be added to getAttributes() too.

neomerx commented 5 years ago

@lindyhopchris added :wink:

lindyhopchris commented 5 years ago

Nice, thanks!

neomerx commented 5 years ago

Released in v4.

lindyhopchris commented 5 years ago

Great, thanks.

neomerx / json-api