Add equals/hashCode to schema types#28
Conversation
… every implementation - according to the spec no two types can have the same name - looking up possible implementations of interfaces relies on .equals, when used with DI pointer equality breaks
|
Thanks for this PR! I will comment on specific code issues. |
There was a problem hiding this comment.
We talked about this, the real question is here, if we have to return null values.
There was a problem hiding this comment.
I don't think that GraphQLModifiedType implementations are ever asked for their name.
Of course that's a little bit hard to see from the interface hierarchy, because the base interface declares getName().
Technically we could return anything, it's never used anywhere. Of course that does not hinder any client to ask for name. The reference implementation uses GraphQLNamedType for this, I guess if we did that it would be clearer. Not sure if it's really worth it, though.
- push getName() implementation and property down - modified types return null for getName - equals/hashcode pushed down as well, modified types delegate to wrappedType
|
It looks like #85 is somewhat related to this PR, unless I am missing something? |
|
Yes, this is the "compare the names in equals" version implementation of #85. I would suggest to add two tests for every type, to show that equals and hashCode are working as intended. Also equals uses And I think it is only a first step because there might code which uses |
|
This fix (or a similar one that properly implements hashCode and equals) is actually needed to properly resolve fragments on union types when using graphql-java-servlet. Currently, |
|
Just popping in, because I received an email today about a comment I can't find now: I'm currently not actively using/trying to use GraphQL and have no spare cycles do maintain this. |
|
@kroepke cool, thanks for checking in!
Yes, this issue is mentioned at the bottom of #205 (comment): it would be useful if the TypeResolver interface provided an instance of the schema in the resolution method. Otherwise, you need the schema somehow available in the TypeResolver's scope, which might require more work on your part. But yes, as you see, the schema definition system as it stands is all about simplicity and therefore uses reference equality/identity. Not great, but at least simple. I, too, would prefer a value-based schema system. But, I am not really comfortable with a hashCode/equals that's based entirely on one field like "name". |
|
I am closing this Issue: The main problem here is that a schema contains not only data but also logic ( |
In order to compose a schema from a set of independently constructed types and interfaces, all GraphQLType implementations should provide a equals/hashCode method.
I've chosen the
nameproperty here, because:SchemaUtil could be converted to use
Set<GraphQLType>in several places instead ofMap<String, GraphQLType>but this is not part of this PR.