Replies: 2 comments 8 replies
-
|
We had discussions like this before. One fundamental problem is that a GraphQLSchema is immutable. Allowing arbitrary metadata would break that. I think we were open to add something like Map<String,String> to it, which would not break immutability. Cc @bbakerman |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, that is totally understandable. For the use case we have right now, it's more of a convenience thing. We are basically serializing and deserializing between a directive and a Kotlin data class that is easier to work with programatically to attach metadata to fields and types. This directive is not used by anyone to define the schema, its metadata added during a processing step of the schema. The other option would be to keep that state outside of the schema objects. |
Beta Was this translation helpful? Give feedback.
-
|
Thinking out loud here, I'm trying to see if it would be any different to have such a field on all |
Beta Was this translation helpful? Give feedback.
-
|
yes you right that we could say: this is not our business, you are on your own when you transform or change or copy a GraphQLSchema. The problem is that this is weaker than the guarantee we give so far, but also transforming/modifying a GraphQLSchema is a real use case vs transforming an ExecutionInput is not really. And people will misuse this, believe me. We will end up with issues asking why the I see two ways forward:
One hacky way to actually store arbitrary data today is to misuse custom scalars: GraphQLScalarType hack = GraphQLScalarType.newScalar().name("HACK").coercing(new Coercing<Object, Object>() {
@Override
public @Nullable Object serialize(@NotNull Object dataFetcherResult) throws CoercingSerializeException {
return null;
}
@Override
public @NotNull Object parseValue(@NotNull Object input) throws CoercingParseValueException {
return input;
}
@Override
public @Nullable Object parseLiteral(@NotNull Object input) throws CoercingParseLiteralException {
return null;
}
}).build();
Map<String, Object> metadata = new LinkedHashMap<>();
GraphQLObjectType query = newObject().name("Query")
.field(newFieldDefinition()
.name("hello")
.type(Scalars.GraphQLString)
.argument(newArgument().name("notUsed").type(hack).defaultValueProgrammatic(metadata)))
.build();
GraphQLSchema schema = newSchema()
.query(query).build();Here we define a custom scalar to hold the metadata map and add it with |
Beta Was this translation helpful? Give feedback.
-
I think the concern has been about object equality. We use it a lot in areas like schema transformers where the visitors and backing zipper code can use object equality direct. In fact we declare this in the So the concern was a "floating" open context object would break that. Now as you say if we had then we could document it as "all bets are off - stick whatever you like in here but its not immutability and its not part of the equality contract of the object. ps if we did this I think we should use
One thing I can see is that by putting a context on schema elements (and maybe the schema itself) then you solve some lifecycle problems. The metadata for a schema and its elements is tied to the lifetime of that schema - where as if you externalise it in some other parallel object you need manage that yourself as a library consumer |
Beta Was this translation helpful? Give feedback.
-
Are you saying users of the SchemaTransformer could expect the "context" to be deep copied when running a transform vs being at best referenced?
If the user application is modifying context elements on a GraphQLSchema that is transformed, it's on them. The same way someone could modify the same value at For (1) is a "smidge" better since it solves the lifecycle issue and it doesn't hijack the directives, but as @xuorig said, it forces serialization/deserialization. Improvement nonetheless. For (2) is interesting since it does the opposite, no need to serialize but where would the object actu
10BC0
ally live?
Yes! Exactly what I had in mind.
Yes we did not create such a construct and most of our code pass around |
Beta Was this translation helpful? Give feedback.
-
|
After giving it another thought and chatting with @bbakerman and @dondonz I believe that any metadata tracked outside of elements is not a good experience. The lifecycle and "scoping" of the data is most likely per element. Tracking it outside of a GraphQLSchema puts a big burden on the user and maybe even more important, often you only have a GraphQLSchemaElement available (e.g. visitor or other APIs). Adding first class support for data tracked outside of the schema everywhere is likely not feasible. But just adding some data construct like Map<String,Object> or even GraphQLContext is also not great as I fear the abuse or misuse especially when a Schema or GraphQLElement is transformed. I propose to explicitly encode the behavior when an element is transformed and at same time leave it to the user to decide what exactly transforming or coping metadata means. I am thinking about an interface like this to be added to every element: interface SchemaElementMetadata {
Object transform();
Object getData();
}
public interface GraphQLSchemaElement {
SchemaElementMetadata getMetadata();
}Additionally we would offer some default implementations like While not perfect, it would enforce an explicit thinking about your metadata behavior, which I think is good enough. What do you think? @bbakerman @xuorig @tinnou |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Extending GraphQL schema element objects with custom metadata can be really useful. For example, back a few years ago it was possible to set arbitrary metadata on GraphQL Ruby elements using its
metadatapropertyfield.metadata[:visibility] = :hidden. (Since then this has changed and is now possible through inheritance)Most of us probably use directives (applied directives 😉 ) to do so at the moment and that makes sense when building schemas using the SDL and where this metadata is defined by the schema builder. However there are still use cases where metadata needs to be applied but a directive is not necessary or wanted:
Would you be open to an extension/metadata system for
GraphQLSchemaElementobjects? One possible downside is that this may introduce 2 ways of doing similar things for some folks, but I think this may lead to a better extension system than serializing / deserializing metadata from a GraphQL directive. Let me know what you think 👂cc @tinnou
Beta Was this translation helpful? Give feedback.
All reactions