[GraphQL] Use same Type for Item and Collection item if possible

[lukasluecke]

If there are no differences in the serialization for Item and Collection items of a resource, we should use the same type - otherwise things get unnecessarily complicated (e.g. Fragments would have to be duplicated).

Currently it also breaks several "smart" caching systems that rely on the __typename.

This would be similar to how we handle the Type used for Mutations (GraphQl/Type/TypeBuilder.php:104)

Related to #3015.

[lukasluecke]

@alanpoulain @mahmoodbazdar Since you were involved in this change, what do you think?

[mahmoodbazdar]

@lukasluecke Allowing user to have different serialization group for items and collections was one of the reasons for this change.
One solution may be to add query back to operations and it apply to both items and collections and if users defined item_query and collection_query we override query settings.

[jamesisaac]

Regarding the serialization side, is the main use-case different permission levels meaning different fields visible for different users?

Is so, the recommended approach I have seen suggested by the Facebook team, is keeping the type the same, but having some of the fields come back as null if the user isn't authorised to see them (link). (Maybe this feature could somehow be provided by API Platform??)

Perhaps there are some situations where a different type would be preferred, but I think in the majority of cases keeping it simple with a single type per entity might be more desirable?

[lukasluecke]

@mahmoodbazdar Did you check the way we handle the Mutation Type? My suggestion is to only use the different type, if there are actually differences in the serialization configuration.

I think in the majority of cases you would have the same fields, so it doesn't make sense to force this complexity on to everyone just for the few exceptions. And if you actually do want different fields, you still can, and then the additional complexity is inevitable, but only for this single resource.

[lukasluecke]

@jamesisaac That also sounds interesting - but then we would have to modify the Type to make all possibly inaccessible fields nullable. I think this might be best suited for a separate discussion / issue 🙂

[mahmoodbazdar]

@lukasluecke You are right.I think we should have done that. I think its a small change. we should update it here:

core/src/GraphQl/Type/TypeBuilder.php

Lines 64 to 72 in 0354379

	if ('item_query' === $queryName) {
	$shortName .= 'Item';
	}
	if ('collection_query' === $queryName) {
	$shortName .= 'Collection';
	}
	if ($wrapped && null !== $mutationName) {
	$shortName .= 'Data';
	}

and check if serialization groups are different.
I can create pull request for that(Or you can do that yourself if you need it quickly)

[lukasluecke]

@mahmoodbazdar Yes, that's what I was thinking as well 🙂 If you could create the PR that would be great 🙌

[mahmoodbazdar]

@lukasluecke OK, I'll create PR this weekend🙂

[lukasluecke]

Just noticed another (bigger) issue with the current implementation - it also breaks several "smart" caching systems that rely on the __typename (e.g. in Apollo, Urql).

If I have a list of resources, the items have the Collection type - If I now update a single resource with a mutation, the result I get has the Item type, which means that the cache doesn't know they relate to the same resource.

I think this should be reason enough to also add a big warning about this in the Docs - you could probably solve this by overriding the cache key generation logic (I don't use Apollo anymore, but it would probably be `dataIdFromObject´).

[jamesisaac]

@lukasluecke Actually after reading this comment: #3040 (comment) I think the technique I described would already be possible, if the auth-related serialization groups are added with the Context Builder.

I'm just thinking the technique allowed with #3015 (and also now recommended in the docs) is maybe an anti-pattern... is there really a strong use-case where you'd want different types for the same entity on collection vs item? The example given in api-platform/docs#863 (admin vs user view) is probably better solved with the Context Builder making fields null for non-admin?

Like you've just commented above, it breaks the assumptions most GraphQL libraries make -- one of the major points of GraphQL is that you can yourself choose exactly which data you want in different queries (say a detailed "item" view vs a smaller "collection" view), that's not something that needs to be enforced at schema level.

[lukasluecke]

@jamesisaac I don't have a strong opinion about the general use-case at the moment - I just want this to be opt-in instead of breaking "everything" without a reason 😅

I think splitting the operations makes sense, because this way you can control if they are registered at all (e.g. you might not even want to provide access to some resources as a collection, only individual items?).

Following the solution we chose for the mutations (as indicated in the issue description) would allow people that have this need of separate serialization settings to fulfill it (with the caveats noted in my last comment), but not affect everyone else. Even the use case for different types returned from mutations might be an "anti-pattern", but it's certainly one that might / did arise.

[alanpoulain]

As @lukasluecke said, I think having a way to distinguish collection and item at the schema level can be useful.
For instance, having null fields for the collection if you are not authorized can be misleading. Having an authorization error directly is better IMO.

For the serialization groups, maybe @jamesisaac is right, I'm not sure having different fields is a good idea since we could return null fields. But it would mean having nullable fields, even for an item whereas it's not necessarily needed.

Implementing different types if the serialization differs (and document the warning about the cache) is not very complicated and it's similar to this code:

core/src/GraphQl/Type/TypeBuilder.php

Lines 100 to 104 in 0354379

	$queryNormalizationContext = $resourceMetadata->getGraphqlAttribute($queryName ?? '', 'normalization_context', [], true);
	$mutationNormalizationContext = $resourceMetadata->getGraphqlAttribute($mutationName ?? '', 'normalization_context', [], true);
	// Use a new type for the wrapped object only if there is a specific normalization context for the mutation.
	// If not, use the query type in order to ensure the client cache could be used.
	$useWrappedType = $queryNormalizationContext !== $mutationNormalizationContext;

This way, it allows the user to do what it wants (and by default it will work correctly with Apollo).

I would not like to reintroduce a query key, and keep item_query and collection_query.

[lukasluecke]

@alanpoulain That's exactly what I was suggesting, and I think it's what @mahmoodbazdar will be doing in his PR 👍 This will solve the immediate issues for everyone that doesn't need different serialization groups / fields. Everything else I think we can tackle later, as it would be more opt-in behaviour, and could be documented as such (with a mention of the possible problems).