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

1670 repeatable directives #1675

Merged
merged 6 commits into from
Jan 6, 2023
Merged

Conversation

t1
Copy link
Collaborator

@t1 t1 commented Jan 5, 2023

No description provided.

t1 and others added 3 commits January 4, 2023 11:15
…irectives to be repeatable. Update the federation annotation documentation and fix some deprecations.
@t1 t1 requested a review from a team as a code owner January 5, 2023 13:16
t1 added 2 commits January 5, 2023 14:17
…tives to be repeatable. Update the federation annotation documentation and fix some deprecations. (squashed)
@t1 t1 force-pushed the 1670-repeatable-directives branch from 6c04b33 to 569f9a7 Compare January 5, 2023 13:41
@t1
Copy link
Collaborator Author

t1 commented Jan 5, 2023

Strange: DynamicClientSSLTest.serverAuthentication_correctTruststore fails with a timeout. @jmartisk: any idea?

@t1 t1 requested a review from jmartisk January 5, 2023 14:05
@jmartisk
Copy link
Member

jmartisk commented Jan 5, 2023

I've seen this too, it seems to have started happening intermittently (like in 1 in 5 runs maybe). I need to look into why.
I restarted the job, perhaps it will pass this time.

@jmartisk
Copy link
Member

jmartisk commented Jan 5, 2023

Ok it passed this time. The WildFly test failure is expected because of the changes we're introducing here, it will need an update of the test in the wildfly-graphql-feature-pack repo.
I'll look a bit more into the changes tomorrow, but it looks good so far

@t1
Copy link
Collaborator Author

t1 commented Jan 6, 2023

Now the FederationTestCase.testFederationMetadataInSchema fails, because the directive changed. I tried to fix it, but a local build fails, so I couldn't test it. Line 60 should read: .body(Matchers.containsString("type Product @extends @key(fields : \"id\")"));.

@jmartisk
Copy link
Member

jmartisk commented Jan 6, 2023

Yeah I can update that when upgrading the feature pack's codebase to a version that contains this change, thanks.
One of your commit messages says that you're updating the documentation, but I don't see that, could you add that?
See https://smallrye.io/smallrye-graphql/2.1.0.Beta1/federation/ - we need to, at least, update the schema snippet at the end

@t1
Copy link
Collaborator Author

t1 commented Jan 6, 2023

The documentation update was only the descriptions of the federation annotations. I'll update the actual docs now; thanks reminding me.

@@ -1,6 +1,6 @@
# Federation

To enable support for [GraphQL Federation](https://www.apollographql.com/docs/federation), simply set the `smallrye.graphql.federation.enabled` config key to `true`.
Support for [GraphQL Federation](https://www.apollographql.com/docs/federation) is enabled by default. If you add one of the federation annotations, the corresponding directives will be declared to your schema and the additional Federation queries will be added automatically. You can also disable Federation completely by setting the `smallrye.graphql.federation.enabled` config key to `false`.
Copy link
Member

@jmartisk jmartisk Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabled by default is bad, IMO.
On the Quarkus side, I've already had a user complaining that they suddenly started seeing

type _Service {
  sdl: String!
}

in their application's schema after upgrading the Quarkus version, even though they don't have anything related to Federation in it, and it broke some of their tooling.
My plan is to make Quarkus detect automatically whether federation should be enabled by scanning through the application for any annotations from io.smallrye.graphql.api.federation, and enabling if found. Of course, you will be able to override this automatic enabling using configuration.

This behavior is Quarkus specific though. WildFly can probably implement something similar if we want. So I'm not sure what to write in the general SmallRye documentation, because it potentially depends on the runtime that you're using. If we make both Quarkus and WildFly behave this way, I would say we can describe this automatic enabling in the SmallRye documentation. WDYT?

Copy link
Member

@jmartisk jmartisk Jan 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So your current text is kinda correct in the sense that if you add a Federation annotation, it will appear in the schema, but it's wrong to say that "Federation is enabled by default", because we don't want to include the type _Service stuff by default. And we won't include federation directive type declarations in the schema either- I have submitted #1678 to address this (don't worry that the PR is to a different branch, I will forward/back-port everything once we merge this and that).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original suggestion to "enable" it by default was from @phillip-kruger, actually... where "enable" means automatic scanning, so the _service and _entities queries only appear when necessary. I thought this was already so... also in JEE containers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, with Quarkus, that stuff appears always by default, even when the application doesn't have any Federation in it. I am going to fix that.
Ok, re-reading your text after this clarification, I think it is fine. It will be enabled in the sense that if you add an annotation, you will get directives and queries. At least this is how it will work after some adjustments in Quarkus+WildFly.

@jmartisk jmartisk merged commit 2c0a6d0 into smallrye:main Jan 6, 2023
@jmartisk jmartisk added back-port This needs to back-port to the 2.0.x branch forward-port-mutiny2 labels Jan 6, 2023
jmartisk pushed a commit to jmartisk/smallrye-graphql that referenced this pull request Jan 6, 2023
* maven-cache the github setup-java action

* fix smallrye#1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations.

* smallrye#1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations. (squashed)

* smallrye#1670: update documentation
jmartisk pushed a commit that referenced this pull request Jan 6, 2023
* maven-cache the github setup-java action

* fix #1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations.

* #1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations. (squashed)

* #1670: update documentation
jmartisk pushed a commit to jmartisk/smallrye-graphql that referenced this pull request Jan 6, 2023
* maven-cache the github setup-java action

* fix smallrye#1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations.

* smallrye#1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations. (squashed)

* smallrye#1670: update documentation
@jmartisk jmartisk removed the back-port This needs to back-port to the 2.0.x branch label Jan 6, 2023
jmartisk pushed a commit that referenced this pull request Jan 6, 2023
* maven-cache the github setup-java action

* fix #1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations.

* #1670: make `@Key` and `@Provides` repeatable and allow directives to be repeatable. Update the federation annotation documentation and fix some deprecations. (squashed)

* #1670: update documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants