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

Support absolute and relative URLs #853

Merged
merged 16 commits into from
Jul 11, 2022
Merged

Support absolute and relative URLs #853

merged 16 commits into from
Jul 11, 2022

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Jul 7, 2022

For each of the 4 supported UI projects, this changes the endpoint paths from PathString to string and allows for the following:

  • Fully qualified URL -- e.g. https://localhost/graphql
  • Absolute URL -- e.g. /graphql
  • Relative URL -- e.g. graphql

For subscription support for the latter 2 scenarios, it will take the current URL to create a path with ws:// or wss:// rather than http:// or https:// respectively.

Replaces:

Fixes:

  • URLs are properly encoded for embedding inside a <script> tag

New feature:

  • Allows configuring RequestCredentials in GraphiQL and Voyager

Todo:

  • Manually test fully qualified urls (see below code snippet)
  • Test relative urls (use in MultipleSchema sample)
  • Test absolute urls (used in all samples)

@Shane32 Shane32 added the enhancement New feature or request label Jul 7, 2022
@Shane32 Shane32 added this to the v7 milestone Jul 7, 2022
@Shane32 Shane32 self-assigned this Jul 7, 2022
@Shane32
Copy link
Member Author

Shane32 commented Jul 8, 2022

Working sample of normal and subscription support for fully qualified urls:

app.UseGraphQLAltair(
    new AltairOptions { GraphQLEndPoint = "https://beta.pokeapi.co/graphql/v1beta", SubscriptionsEndPoint = "wss://beta.pokeapi.co/graphql/v1beta" },
    "/poke/altair");
app.UseGraphQLGraphiQL(
    new GraphiQLOptions { GraphQLEndPoint = "https://beta.pokeapi.co/graphql/v1beta", SubscriptionsEndPoint = "wss://beta.pokeapi.co/graphql/v1beta" },
    "/poke/graphiql");
app.UseGraphQLPlayground(
    new PlaygroundOptions { GraphQLEndPoint = "https://beta.pokeapi.co/graphql/v1beta", SubscriptionsEndPoint = "wss://beta.pokeapi.co/graphql/v1beta" },
    "/poke/playground");
app.UseGraphQLVoyager(
    new VoyagerOptions { GraphQLEndPoint = "https://api.spacex.land/graphql/" },
    "/spacex/voyager");

Note: Request credentials must be set to Omit / SameOrigin or else requests will be blocked by CORS. Altair seems to default to Omit/SameOrigin and does not appear to be easily configurable, and Playground uses Omit/SameOrigin as default. Support was added to configure RequestCredentials for GraphiQL and Voyager, with SameOrigin as the default. Previous default was Include, but then again, other origins were not supported at all.

SameOrigin is default behavior for fetch.

(edited)

@Shane32 Shane32 requested a review from sungam3r July 8, 2022 14:40
@Shane32
Copy link
Member Author

Shane32 commented Jul 8, 2022

@sungam3r This is ready for review. Everything works and has been tested. An alternative way to code this functionality is to have it compute the relative urls and stuff server-side. Perhaps with Uri rather than string. Or create a Uri from the string.

Shane32 and others added 2 commits July 9, 2022 16:49
Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
app.UseGraphQLPlayground(
new GraphQL.Server.Ui.Playground.PlaygroundOptions
{
GraphQLEndPoint = "/cats/graphql",
SubscriptionsEndPoint = "/cats/graphql",
GraphQLEndPoint = "../graphql",
Copy link
Member

@sungam3r sungam3r Jul 9, 2022

Choose a reason for hiding this comment

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

I don't know JS and this seems strange to me. I expected that ../graphql for /cats/ui/playground is /cats/ui/graphql like things for file paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, “graphql” would be the sibling file while “../graphql” is GraphQL within the parent folder. Note that no JS parsing is done to the GraphQL endpoint; only the subscription endpoint. And as both work, it shows that the code matches the browser interpretation of “../graphql”

Copy link
Member Author

Choose a reason for hiding this comment

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

Does that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

I don't get it yet. What is a "parent folder" for /cats/ui/playground?

Copy link
Member Author

Choose a reason for hiding this comment

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

URL Path File img.jpg resolves to ../img.jpg resolves to
/ / (none) /img.jpg n/a
/index.html / index.html /img.jpg n/a
/ui/ /ui/ (none) /ui/img.jpg /img.jpg
/ui/index.html /ui/ index.html /ui/img.jpg /img.jpg
/ui/playground /ui/ playground /ui/img.jpg /img.jpg

You can similarly apply graphql in place of img.jpg:

URL Path File graphql resolves to ../graphql resolves to
/ / (none) /graphql n/a
/index.html / index.html /graphql n/a
/ui/ /ui/ (none) /ui/graphql /graphql
/ui/index.html /ui/ index.html /ui/graphql /graphql
/ui/playground /ui/ playground /ui/graphql /graphql

Copy link
Member Author

@Shane32 Shane32 Jul 9, 2022

Choose a reason for hiding this comment

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

The above samples are just typical samples of how URLs work within any HTML. There is nothing special about GraphQL or Playground that would affect how URLs are interpreted. The only reason we need special code at all is because fully-qualified URLs are required for WebSocket communication. So while we can pass relative URLs directly to fetch, we cannot use relative URLs for WebSocket connection requests. So now for fetch (for POST requests), we just pass the unmodified relative (or absolute) URL while for WebSockets, we convert relative URLs to fully-qualified URLs.

Also, by testing ../graphql for the /cats/ui/playground endpoint, we can be sure that the code works properly. If it navigated up too many parents, /graphql would be attempted and would fail. Likewise if it did not navigate up enough parents, /cats/ui/graphql would be attempted and would fail. It is also important to notice that both query/mutations work (over POST via the unmodified relative URL), and subscriptions (over WebSockets via the modified URL).

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the reason fully-qualified URLs are required for WebSocket communication is because the protocol is "ws" or "wss" rather than "http" or "https". Since the relative path of ../graphql for https://localhost/cats/ui/playground resolves to https://localhost/cats/graphql, connection fails because it has the protocol of "https" rather than "wss".

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I think I get it, I was looking at /cats/ui/playground as three-folder path cats->ui->playground so one step up would be cats->ui but playground is not a folder, it is a resource (file). Thanks.

Regarding that comment:

The only reason we need special code at all is because fully-qualified URLs are required for WebSocket communication. So while we can pass relative URLs directly to fetch, we cannot use relative URLs for WebSocket connection requests. So now for fetch (for POST requests), we just pass the unmodified relative (or absolute) URL while for WebSockets, we convert relative URLs to fully-qualified URLs.

I beleive it's worth to put it somewhere into the code.

Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
@codecov-commenter
Copy link

Codecov Report

Merging #853 (9970f6a) into develop (873b21b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #853   +/-   ##
========================================
  Coverage    95.64%   95.64%           
========================================
  Files           41       41           
  Lines         1976     1976           
  Branches       332      332           
========================================
  Hits          1890     1890           
  Misses          47       47           
  Partials        39       39           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 873b21b...9970f6a. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants