Entity Representation Mappers #1669
Conversation
There was a problem hiding this comment.
I think we should keep the implementation backwards compatible, i.e. we need to keep the checks that the parameters allowed by the entity fetcher are either a Map, keep the current behavior, or the type that the converter expects.
Aside from this this is a good implementation imo, an extension of this would be to provide a default representation mapper that is based on Spring native type conversion example here.
| } | ||
| val mapper = entityRepresentationMapperRegistry.mappers[typename]; | ||
|
|
||
| val mappedValues = if (mapper != null) mapper.second.invoke(mapper.first, values) else values; |
There was a problem hiding this comment.
I think there should be a check if the parameter is assignable to the actual mapped value type. Falling back to the raw value if not. This will make it backward compatible with Data Fetchers that assume they are going to receive a Map.
| @@ -87,15 +93,15 @@ open class DefaultDgsFederationResolver() : | |||
| val fetcher = entityFetcherRegistry.entityFetchers[typename] | |||
| ?: throw MissingDgsEntityFetcherException(typename.toString()) | |||
|
|
|||
| if (!fetcher.second.parameterTypes.any { it.isAssignableFrom(Map::class.java) }) { | |||
There was a problem hiding this comment.
I think we should keep this check in some manner. i.e.
- Do we have a mapper for this representation and does the entity fetcher provide a parameter that will be compatible with the mapped type. Then map the representation and pass the result.
- If the above is not
true, fallback to the current behavior. Checking if there is at least one parameter that defines a mapMap::class.java. Pas the values as is, keeping the current behavior.
There was a problem hiding this comment.
Yeah, that's a good point. I think it might need to be adjusted a bit to improve visibility into type expectations, though, otherwise it could hide incorrectly implemented @DgsEntityRepresentationMapper implementations.
I was thinking this:
- If we do have a representation mapper for this representation...
a) check if the mapper accepts a parameter of typeMap::class.java, and throw anInvalidDgsEntityRepresentationMapperif it does not.
b) check if the entity fetcher provides a parameter that is compatible with the mapped representation type, and throw anInvalidDgsEntityFetcherindicating that it should accept the mapped type if it does not. - If we do not have a representation mapper for this representation...
a) check if the entity fetcher provides a parameter that is compatible withMap::class.java, and throw anInvalidDgsEntityRepresentationMapperindicating that it should accept aMap::class.javaif it does not (current behavior).
Since we can assume that no one will be using the new @DgsEntityRepresentationMapper annotation, this should still be backwards compatible with the current version, if I'm understanding correctly. @berngp thoughts?
There was a problem hiding this comment.
b) check if the entity fetcher provides a parameter that is compatible with the mapped representation type, and throw an
InvalidDgsEntityFetcherindicating that it should accept the mapped type if it does not.
Not sure that is the best approach, suggest the check is as follows...
- Check if the entity fetcher supports a
Mapas representation.
1.1 Proceed with just passing the representation as a map. i.e. as is. - If not, fetch the type of the parameter expected by the entity fetcher as representation.
- Find the Representation Mapper that applies to the
__typenameas well as the type expected by the entity fetcher's parameter. i.e. the Representation Mapper should have a method that tells us if they apply or not for the__typenameand the expected class.
3.1 If there are candidate mappers, proceed and map the type and call the entity fetcher. Log which mapper was used. - If there are no Representation Mapper, throw an
InvalidDgsEntityRepresentationMapperthat specifies the__typename, the expected entity fetcher parameter type, and the class and method of the entity fetcher.
This will allow us to roll the feature out without affecting any customers. Even if the Representation Mapper is provided, it will only apply for the Entity Fetchers that are ready and explicitly expect the representation as a type other than a Map.
There was a problem hiding this comment.
I like that approach and will take a stab at applying it. Thanks for the feedback so far! 😄
There was a problem hiding this comment.
@wbaldoumas - Thanks so much for all the work so far. A few of us were able to take a look and discuss the approach.
We decided to not go ahead with this PR because 1) Fixing the issue of scalar parsing would be better handled within the framework ideally. We don't quite yet have a solution but will need to explore it further. 2) We agree it would be nice to have a better way of making the representation available readily for the user within the fetcher, we would like to consider a mechanism that leverages existing code conversions, similar to @InputArgument if possible.
We also agreed that the solution proposed in this PR would be better handled within a library outside of the framework. We really appreciate all the effort and for kicking off the ideation process. Ultimately this is a current gap in the framework, so we will prioritize adding support for this.
There was a problem hiding this comment.
We will close out this PR and post an update when we have the new feature.
There was a problem hiding this comment.
@srinivasankavitha that works for me. Thanks so much for taking a look and for the collaboration so far.
No worries on dropping this PR. I'm glad that there are more eyes on this problem at this point and that some solution for it will be getting prioritized. It has also been great to poke around at the internals here and get an understanding of how some of this works under the hood. 😄
I'll keep an eye out on the issue @gwardwell posted to track progress.
|
Any thoughts @srinivasankavitha & @kilink? I think this will also be useful internally, specially if developers combine it codegen. |
Pull request checklist
first
Pull Request type
Changes in this PR
Works to address #1569.
Allow users of DGS to optionally provide
@DgsEntityRepresentationMapperimplementations that let them encapsulate and abstract logic which can map from theMap<String, Any>representations that are passed to their@DgsEntityFetcherto concrete representation classes instead.Leveraging this should lead to cleaner and leaner
@DgsEntityFetcherimplementations, and the@DgsEntityRepresentationMapperimplementations can be shared and reused where applicable.Note: the validation that ensured that the
@DgsEntityFetchermethod parameter was of typeMaphas been removed, since this method parameter may now be a custom class instead.Alternatives considered
A more opinionated implementation of this was considered, which defined an
EntityRepresentationMapperandEntityRepresentationinterface, which would allow for stricter typing and some additional validations. At the moment, this felt too constrictive and doesn't align with the conventions that are in place for@DgsEntityFetcherthat DGS users would be used to.