Replies: 9 comments 28 replies
-
Well, looks good. I would like to make a few re-naming suggestions. The __Metadata type: type __Metadata {
directive: __Directive!
values: [__MetadataValue!]
}
type __MetadataValue {
name: String!
value: String!
} What we have here is ref to Directive, and its arg values - I think AppliedDirective would be more intuitive: type __AppliedDirective {
directive: __Directive!
values: [__ArgValue!]
}
type __ArgValue {
name: String!
value: String!
} And on introspection types, I suggest the field name for AppliedDirectives: type __Field {
# ...
appliedDirectives: [__AppliedDirective!] # instead of 'metadata'
} Previously, I suggested to move away from the term 'custom metadata'; in this post I explain why; Now, wait a minute - what we have here now, with this renaming is the appliedDirectives approach that I am arguing for! Seriously, I am happy that you came up with practically the same thing. Let's push for this approach together! I will follow up with some other comments regarding few other aspects in your suggestion. |
Beta Was this translation helpful? Give feedback.
-
When we implemented the Applied Directive concept with Hot Chocolate we added an internal switch to the directive allowing the user to expose them or not... essentially we modeled that as a modifier that could have the value public or internal. This is essentially what the metadata keyword is doing now. There is still one downside in the overall design here, which exists any way for input fields, and that is that values still need to be parsed twice... essentially the result must be parsed, and in a second round, the values must be parsed. I do not think that is necessarily an issue we need to solve since this issue already exists with the default value, but it's good to point it out and reflect on it. The things I like about this approach and the applied directives approach are that it is a minimal change to unlock metadata and many use-cases with it. In general, we should try to unlock features like this with an incremental change rather than with a big leap, as they allow for faster iteration and fewer risks to current implementations. In general, I think that we should take directives and improve them rather than introducing something new here. Regarding the naming, if we distinguish certain kinds of directives as metadata, then we should also use that in the introspection as applied directive would indicate that I can access all applied directives, which is not the case. So, I am in favor of the __Metadata type name. |
Beta Was this translation helpful? Give feedback.
-
metadata vs appliedDirectivesI am still strongly in favor of 'appliedDirectives':
Naming is super-important! And remember Occam's razor: "entities should not be multiplied beyond necessity" Need to parse returned arg values from strings.I actually do not see it as a big problem, for now at least. It depends on what format we serialize the values. GraphQL format - yes, it would be problematic. The client does not have a GraphQL parser, by default. But if we serialize as Json - no problem, the client already has Json deserializer, all responses from server are json. All it needs to do with argValue string is unescape it (like all escaped double quotes) and then use json deserializer to read it as Type that is known from directive definition. One minor inconvenience with Json format is that in Graphiql, when you run introspection query, the arg values will show as escaped Json, which would be a bit ugly for complex input types. But I think it's just inconvenience. No impact on runtime; 'parsing twice' - well, I would expect client would run introspection queries once, not as routine, so runtime perf impact would negligible. We can plan for the future to improve it. The best way is to introduce Any scalar (or struct), which would mean: for value of this field expect some Json subtree, as Json, not escaped string. Json value can be a simple value (123) or complex subtree for input type. The client can deserialize the subtree into an object if the type is known (like in our case with arg values). This is for the future, for now we can use Json String. But anticipating the future we can name the field "valueJson", so that we can add "value" in the future: type __ArgValue {
name: String!
valueJson: String! # now
value: Any # added in the future
} So in the future we will have both, and I don't think it's a problem. The client might choose one or another, json tree or json string, depending on situation. 'metadata' keyword in directive declaration and isMetadata fieldWhether we use AppliedDirective or Metadata as basic names, I think the 'metadata' keyword in directive declaration is confusing, better name would be 'public' or 'private', directly expressing the meaning: directive @myCustomMetadata(myValue: String) metadata repeatable on FIELD # original
# vs
directive @myCustomDir(myValue: String) private repeatable on FIELD # suggested I would suggest 'private' keyword, meaning 'do not expose'; by default all public, only a few marked as private. But in fact, I would really argue to have NO keyword at all. Just look at the situation closely. We declare a private, non-exposed directive, return it's declaration in __Schema.directives list. We say to the client: this directive is defined by the server. But we won't show you where it's applied, it is a secret. Then - what's the point?!!! Why show the dir definition at all? How it should work then with 'privates'? Just like Michael described with appliedDirective implementation in HotChocolate - in the class that represents the directive in code, there is an internal flag that indicates if it is public, exposed or not. It is internal, server-only thing, and the dir is not exposed at all, all done by server code. So the spec and introspection do NOT need any 'private' keyword or 'isPrivate' field in intro type. I hope I explained it clear enough. Things private to server are not of concern of the spec! This is a clear illustration to the point I was arguing in the WG meeting, regarding GraphQL formal definition and scope. If we established that GraphQL is 'communication protocol' then it would clearly set upfront that server-only private things are OUT of spec. And we would not have to even start discussing things like 'how to indicate that it is server-side only private thing'. Please think about this in this light. |
Beta Was this translation helpful? Give feedback.
-
Well, I do not agree that "exposed" or "public" flag on directive helps in this case. As far as I see it, we are talking about this setup: (client) <-> (AFS: Apollo Federation Server) <-> (MGS: Multiple GraphQL Servers) (based on my understanding of how federation works): |
Beta Was this translation helpful? Give feedback.
-
Just want to remind about huge disadvantage of GraphQL encoding of values - the need for GraphQL parser on the client. That is technically not a big deal for defaultValues - most default values are primitive scalar values, and their string representations are the same for Json or GraphQL; these values can be converted simply by using conversion functions available directly in the language (js, something like convertFromString). I guess it's very rare that you have some complex input object with constant default value. In the case here, this will happen way more often, when we have input type as input value for directive. With Json representation, it is trivial, Json deserializer is always there in the client; since any graphQL response is Json. But bringing in GraphQL parser - that might be different story, it a whole separate library, not sure it is readily available for different languages, and you need to integrate it, learn its API etc. So I would suggest that having a light inconsistency with defaultValue is tolerable in this case, for the sake of easier, primitive handling of Json. We can look at how to 'ease' the inconsistency problem, like allow both encodings? or extra defaultValueJson, optional/nullable for now? This in fact would be a relief for clients, since again, Json is easier for them. |
Beta Was this translation helpful? Give feedback.
-
I am not sure if this proposal was just outlining where the "metadata" (aka applied directives) might be applied in introspection but would it be all the places you can put a directive on schema element? type __Schema {
# ...
metadata: [__Metadata!] # Example of this being added to all things.
}
type __Type {
# ...
metadata: [__Metadata!] # Example of this being added to all things.
}
type __Field {
# ...
metadata: [__Metadata!] # Example of this being added to all things.
}
type __EnumValue {
# ...
metadata: [__Metadata!] # Example of this being added to all things.
}
type __InputValue {
# ...
metadata: [__Metadata!] # Example of this being added to all things.
}
|
Beta Was this translation helpful? Give feedback.
-
@leebyron I'm still formulating my opinion about this proposal in general. We can't parse values in GraphQL format without access to
More real-life examples are custom numeric scalars implemented in any major ecosystem (JS, java, C#, etc.) some ecosystems have support for arbitrary long numbers (e.g. graphql-java has GraphQLBigInteger).
Note: if the value goes through scalar's So that means we can't parse the value literals using input type of directive arguments since we don't have access to In general, I just want to emphasize that before this change, GraphQL Literal syntax was only used as input format. As for |
Beta Was this translation helpful? Give feedback.
-
I really like this approach of adding the new In theory, these introspection values could be moved to the metadata approach where the spec reserves the To that end, if we do implement it so that other existing introspect-able info is available, maybe the spec should add this info under the keys |
Beta Was this translation helpful? Give feedback.
-
Sorry for the delay in getting back; I've had a busy August! Though a straightfoward mapping of directives, I feel that this and the The way I see it, there are two main use cases for schema introspection and they're essentially governed by which of the two introspection entry-fields you use:
The
Unfortunately the I've prepared a few slides on some of the use cases I'm particularly excited about that granular parser-less metadata would enable; please have a quick read (it won't take long!): https://docs.google.com/presentation/d/1e6o2kd3fVc_DQH1O8RxJo-idZM0kTOx-q322coNeIIo/present |
Beta Was this translation helpful? Give feedback.
-
Given the revived discussion around schema metadata and their relationship with SDL directives, I'd like to propose a high level direction for this. My goal here is to introduce something with as few additional moving parts as possible. It's intended to be simple rather than robust.
The primary new mechanism is being able to mark an SDL Directive definition as
metadata
and exposing such directive usages via introspection. Given an introspection result you should be able to re-create the SDL with directive usages in the right places.The main downside is that the introspection encoding is simple to the point of requiring post-processing. There is no explicit sub-selection; all metadata gets exposed in introspection. The metadata is not in a structured JSON form, but in arrays of named groups - however this more clearly maps to the way directives are being used. It's up to tools consuming metadata to interpret these as intended.
I'd love feedback on this!
That's it! Let's take a look at an example:
And here's the resulting introspection response (in JSON5) for this particular fragment applying to the
myField
Beta Was this translation helpful? Give feedback.
All reactions