Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Custom Scalar Specification URLs #649

Open
wants to merge 14 commits into
base: master
from

Conversation

@eapache
Copy link
Contributor

eapache commented Nov 20, 2019

This PR contains the specification changes needed for #635.

@sungam3r

This comment has been minimized.

Copy link
Contributor

sungam3r commented Nov 20, 2019

Relates to #633

@@ -146,6 +146,9 @@ type __Type {
# should be non-null for NON_NULL and LIST only, must be null for the others
ofType: __Type
# should be non-null for SCALAR only, must be null for the others
specifiedBy: String

This comment has been minimized.

Copy link
@sungam3r

sungam3r Nov 20, 2019

Contributor

It correlates to some extent with #300 .
If this extension of the introspection schema will be added, it is logical to expect that the proposed in #300 extension
should be also accepted. Now there is a second server directive besides @deprecated, which is provided through introspection as a special property. I have nothing against this, but I believe that we need to provide a way to get user directives through introspection too. It will be a consistent step.

This comment has been minimized.

Copy link
@michaelstaib

michaelstaib Nov 20, 2019

@sungam3r there are some experiments on introspection extensions. Technically you can already today extend introspection but this alway comes with the danger that you violate a future specification.

@IvanGoncharov was experimenting on this one with GraphQL-js. At the moment you can put metadata on almost everything there. We do almost the same with hot chocolate. The next thing would be to allow to have something like extensions on introspection so that there is a point where you can cleanly extend the introspection types.

Doing this through directives brings some challenges since directives, specifically their arguments are input types that could bear challenges to automatically extend the introspection from those.

@IvanGoncharov maybe we should start a new discussion on that?

This comment has been minimized.

Copy link
@sungam3r

sungam3r Nov 21, 2019

Contributor

I have a separate graphql-dotnet branch in which directives are already returned via introspection. We use this forked preview version in production.

this alway comes with the danger that you violate a future specification.

I agree. It’s not scary for us. We need new features now, and not when they (possibly) appear in the specification.

This comment has been minimized.

Copy link
@michaelstaib

michaelstaib Nov 21, 2019

@sungam3r i did mean that more from them perspective. Hc allows extending the introspection already for a couple of versions. But the user that does that has to know that he/she might violate a future spec. We just have a warning in the docs that you can do it but may run into an issue in the future.

```graphql example
scalar Time
scalar Url
scalar Time @specified(by: "https://tools.ietf.org/html/rfc3339")

This comment has been minimized.

Copy link
@andimarek

andimarek Nov 20, 2019

This is a bit "misleading" example because RFC3339 is normally used to describe date and time.
How about scalar DateTime @specified(by: "https://example.com/myDateTime") ?

This comment has been minimized.

Copy link
@andimarek

andimarek Nov 20, 2019

Using a non RFC link would also make it more clear that it is totally fine to come up with your scalar spec.

This comment has been minimized.

Copy link
@michaelstaib

michaelstaib Nov 21, 2019

I think we all agreed to have a specific spec for datetime since it is based on the rfc but more narrow in its implementation. There should be a specific document outlining the rules for this one.

This comment has been minimized.

Copy link
@eapache

eapache Nov 26, 2019

Author Contributor

Yeah, I might just take this out as an example until we have something more useful to link to.

This comment has been minimized.

Copy link
@eapache

eapache Nov 26, 2019

Author Contributor

Actually, if I take it out it gets even more confusing because the other example to talk about in the paragraph above is URL and then it gets really hard to parse whether I'm talking about the URL scalar as an example, or the URL that you provide in the example to link to the specification.

I'll rename this to DateTime for now, with the expectation that when Andi's restricted datetime scalar format doc gets published we'll change the link to that.

@@ -232,13 +235,16 @@ actually valid. These kinds are listed in the `__TypeKind` enumeration.
Represents scalar types such as Int, String, and Boolean. Scalars cannot have fields.
A GraphQL type designer should describe the data format and scalar coercion
rules in the description field of any scalar.
A GraphQL type designer should provide use the `@specified` directive to provide

This comment has been minimized.

Copy link
@andimarek

andimarek Nov 20, 2019

typo: should use the

@andimarek

This comment has been minimized.

Copy link

andimarek commented Nov 20, 2019

great work @eapache

@@ -343,9 +343,17 @@ client-specific primitive for time. Another example of a potentially useful
custom scalar is `Url`, which serializes as a string, but is guaranteed by
the server to be a valid URL.
When defining an additional scalar, GraphQL systems should use the `@specified`
directive to provide an RFC3986-compliant URI pointing to a human-readable

This comment has been minimized.

Copy link
@leebyron

leebyron Dec 5, 2019

Collaborator

I think throughout we can replace RFC3986-compliant URI with just URL

RFC3986-compliant URI could be understood to include an ISBN for an academic paper or something like that, and the goal here is that a person can load this URL in a browser and go read the spec. URL is well understood with a single definition, so there's no need to qualify it further with which IETF RFC you're referring to.

m14t added a commit to m14t/graphql-js that referenced this pull request Dec 6, 2019
This in an implementation for a spec proposal:

* Spec proposal: [[RFC] Custom Scalar Specification URIs](graphql/graphql-spec#649)
* Original issue: [[RFC] Custom Scalar Specification URIs](graphql/graphql-spec#635)

Please let me know if I forgot something.
m14t added a commit to m14t/graphql-js that referenced this pull request Dec 6, 2019
This in an implementation for a spec proposal:

* Spec proposal: [[RFC] Custom Scalar Specification URIs](graphql/graphql-spec#649)
* Original issue: [[RFC] Custom Scalar Specification URIs](graphql/graphql-spec#635)
@@ -146,6 +146,9 @@ type __Type {
# should be non-null for NON_NULL and LIST only, must be null for the others
ofType: __Type
# should be non-null for SCALAR only, must be null for the others

This comment has been minimized.

Copy link
@m14t

m14t Dec 7, 2019

Nitpick: Currently in my draft for the reference implementation graphql/graphql-js#2276, if you don't add the @specified directive to a scalar, specifiedBy will be null.

If that's the correct implementation, then perhaps this wording could be updated to something along the lines of:

Suggested change
# should be non-null for SCALAR only, must be null for the others
# should be a string or null for SCALAR only, must be null for the others

This comment has been minimized.

Copy link
@eapache

eapache Dec 10, 2019

Author Contributor

Hmm, it's a good point but I think I'd prefer just replacing should with can or may.

@eapache eapache changed the title [RFC] Custom Scalar Specification URIs [RFC] Custom Scalar Specification URLs Dec 10, 2019
@eapache eapache force-pushed the eapache:custom-scalar-formats branch from d8ed061 to f72a146 Dec 10, 2019
scalar Time
scalar Url
scalar UUID @specified(by: "https://tools.ietf.org/html/rfc4122")
scalar URL @specified(by: "https://tools.ietf.org/html/rfc3986")

This comment has been minimized.

Copy link
@eapache

eapache Dec 10, 2019

Author Contributor

@leebyron I changed the capitalization here from Url to URL because Uuid looked particularly gross, and the built-in is already ID not Id. It's a minor editorial thing but it's not really related to this RFC so I wanted to make sure it was ok.

This comment has been minimized.

Copy link
@leebyron

leebyron Jan 11, 2020

Collaborator

Seems totally fine to me

Fields
* `kind` must return `__TypeKind.SCALAR`.
* `name` must return a String.
* `specifiedBy` may return a URL or {null}.

This comment has been minimized.

Copy link
@IvanGoncharov

IvanGoncharov Dec 11, 2019

Member

We don't have a URL type so it should be String.

This comment has been minimized.

Copy link
@eapache

eapache Dec 16, 2019

Author Contributor

maybe may return a String (in the form of a URL) or {null}. ?

@IvanGoncharov

This comment has been minimized.

Copy link
Member

IvanGoncharov commented Dec 15, 2019

@eapache bikeshedding: How about changing @specified(by: 'http://example.com') to @specifiedBy(url: 'http://example.com')
Pros:

  • Fewer chances to conflict with existing and previous directives
  • It's easier to figure out that you pass/get URL from the name, e.g. inside introspection we would have specifiedByURL instead of specifiedBy
@sungam3r

This comment has been minimized.

Copy link
Contributor

sungam3r commented Dec 15, 2019

+1 for @specifiedBy(url: 'http://example.com')
-1 for specifiedByURL

This is a personal feelings, specifiedByURL causes some associations with the Hungarian notation, which has its drawbacks.

@eapache

This comment has been minimized.

Copy link
Contributor Author

eapache commented Dec 16, 2019

I'm weakly in favour of @specifiedBy(url:) instead of @specified(by:).

relevant IETF specifications.

```graphql example
scalar UUID @specifiedBy(url: "https://tools.ietf.org/html/rfc4122")

This comment has been minimized.

Copy link
@m14t

m14t Dec 30, 2019

While implementing this in graphql/graphql-js#2276 @IvanGoncharov provided the following comment:

If at some point we decide to add UUID scalar into this library it would be a confusing example.
It's better to use some abstract scalar like Foo and https://example.com/foo_spec.

Question: Do we want to make that same change here as well? If so this likely applies to URL as well, and effects a few locations in this PR.

This comment has been minimized.

Copy link
@eapache

eapache Feb 5, 2020

Author Contributor

Point taken that it could become confusing, but it's not confusing yet, and there is definitely value in having a concrete example that lines up with use cases a lot of actual developers will run into. I'm planning to leave this as is, and we can change it if becomes an issue later.

spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
m14t added a commit to m14t/graphql-js that referenced this pull request Dec 31, 2019
This in an implementation for a spec proposal:

* Spec proposal: [[RFC] Custom Scalar Specification URIs](graphql/graphql-spec#649)
* Original issue: [[RFC] Custom Scalar Specification URIs](graphql/graphql-spec#635)
m14t added a commit to m14t/graphql-wg that referenced this pull request Jan 2, 2020
I've been working on the [implementation](graphql/graphql-js#2276) of the `@specified(by: "")` directive as specified by graphql/graphql-spec#649, and am interested an any feedback or progress on that effort.
@IvanGoncharov

This comment has been minimized.

Copy link
Member

IvanGoncharov commented Jan 8, 2020

Reviewing graphql/graphql-js#2276 I saw another argument for naming argument url instead of by, since by by itself looks weird in code:
https://github.com/graphql/graphql-js/pull/2276/files#diff-71ba52e9c625f826d2b0df2963c8633aR328

m14t added a commit to m14t/graphql-wg that referenced this pull request Jan 9, 2020
I've been working on the [implementation](graphql/graphql-js#2276) of the `@specified(by: "")` directive as specified by graphql/graphql-spec#649, and am interested an any feedback or progress on that effort.
eapache added 9 commits Aug 15, 2019
As Benjie noted, it's a bad idea (though not strictly breaking, if the
spec has simply moved).

Also tidy up the previous paragraph a bit.
They're not quite the same thing, but URL is what we actually mean even
if it's less formally specified.
Not all scalars will have a `specifiedBy` value so "should" was too
strong.
Also fix a few more URI->URL references.
@leebyron leebyron force-pushed the eapache:custom-scalar-formats branch from 1758a1c to cb92ed5 Jan 11, 2020
Copy link
Collaborator

leebyron left a comment

This is looking great.

I think the only thing keeping it from Stage 3 is landing graphql/graphql-js#2276

spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
When defining an additional scalar, GraphQL systems should use the
`@specifiedBy` directive to provide a URL pointing to a human-readable
specification of the data format, serialization, and coercion rules for the
scalar. For example, a GraphQL system providing a `UUID` scalar might link to
RFC 4122, or some document defining a reasonable subset of that RFC. If a
specification URL is present, systems must conform to the described rules.
Built-in scalar types should not provide a URL.
```graphql example
scalar Time
scalar Url
scalar UUID @specifiedBy(url: "https://tools.ietf.org/html/rfc4122")
scalar URL @specifiedBy(url: "https://tools.ietf.org/html/rfc3986")
```
Comment on lines 344 to 355

This comment has been minimized.

Copy link
@leebyron

leebyron Jan 11, 2020

Collaborator

It would be great to elaborate a bit here about how we expect client tooling to use this to provide additional behavior, and maybe even a note about how two GraphQL services with different named scalars specified by the same url should be imbued with the same behavior and conversely two services might have a scalar by the same name but specified with different urls.

This comment has been minimized.

Copy link
@eapache

eapache Feb 5, 2020

Author Contributor

Hmm, that's a good idea. Since I believe this is basically ready to go as-is (once the implementation lands, which should be soon?) I might put that content into a second PR to avoid holding this one up over additional wording quibbles.

spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
spec/Section 3 -- Type System.md Outdated Show resolved Hide resolved
spec/Section 4 -- Introspection.md Outdated Show resolved Hide resolved
@lirsacc lirsacc mentioned this pull request Jan 28, 2020
4 of 9 tasks complete
@michaelstaib michaelstaib mentioned this pull request Jan 29, 2020
3 of 11 tasks complete
eapache and others added 4 commits Feb 5, 2020
Co-Authored-By: Matt Farmer <work@mattfarmer.net>
Co-Authored-By: Lee Byron <lee@leebyron.com>
Co-Authored-By: Lee Byron <lee@leebyron.com>
@eapache eapache force-pushed the eapache:custom-scalar-formats branch from be4d760 to 12f9f21 Feb 5, 2020
@andimarek

This comment has been minimized.

Copy link

andimarek commented Feb 6, 2020

This PR introduces another built-in directives next to deprecated, skip and include.

The question are: (which also affect the other built-ins):

Additionally: are you allowed to redefine specifiedBy? (Same question for the other built-ins) GraphQL.js currently allows redefining deprecated, skip and include. If you allow redefining them the server implementations need to distinguish between the built-ins specifiedBy and the custom specifiedBy.

I lean towards not allowing to redefine them, but this would make this new directive a breaking change for all schemas which already define a specifiedBy directive.

@IvanGoncharov

This comment has been minimized.

Copy link
Member

IvanGoncharov commented Feb 6, 2020

@andimarek Realistically speaking, how many API have specifiedBy directive?
I would say zero, with the exception of implementations motivated by this PR.

I don't think we should increase scope to resolve purely theoretical issues.
My opinion specifiedBy should be treated exactly the same as @if, @skip or @deprecated.

@sungam3r

This comment has been minimized.

Copy link
Contributor

sungam3r commented Feb 6, 2020

I agree with @IvanGoncharov, although such judgments always give a special taste to statements about the priority of backward compatibility.

@andimarek

This comment has been minimized.

Copy link

andimarek commented Feb 6, 2020

@IvanGoncharov I agree with not increasing the scope of this change. But I think we should follow up with some additional clarification how to handle custom directives called specifiedBy, deprecated etc.

Sure it will only affect a very small minority of schemas, but we should nevertheless care.

It could be as simple as saying that custom directives named specifiedBy etc. are not allowed and pointing out it in the release notes for the next spec release that it is a theoretical breaking change. I think this would be sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.