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

Some headers are incorrectly parsed #2660

Open
4 tasks
aaronleopold opened this issue Nov 15, 2023 · 5 comments
Open
4 tasks

Some headers are incorrectly parsed #2660

aaronleopold opened this issue Nov 15, 2023 · 5 comments
Labels
stage/1-reproduction A reproduction exists

Comments

@aaronleopold
Copy link

aaronleopold commented Nov 15, 2023

Issue workflow progress

Progress of the issue based on the
Contributor Workflow

  • 1. The issue provides a reproduction available on GitHub, Stackblitz or CodeSandbox

    Make sure to fork this template and run pnpm generate in the terminal.

    Please make sure the Codegen and plugins version under package.json matches yours.

  • 2. A failing test has been provided
  • 3. A local solution has been provided
  • 4. A pull request is pending review

Describe the bug

I am trying to add an Origin header to command, but my server is picking up an incorrect header. For example, if I run:

graphql-inspector introspect http://localhost:5500/api --header 'Origin: http://localhost:5500/api' --write schema.graphql

The header value that gets passed to my server is almost correct:

http//localhost:5500/api

I believe the issue is here: https://github.com/kamilkisiela/graphql-inspector/blob/master/packages/commands/commands/src/index.ts#L51-L55

To Reproduce Steps to reproduce the behavior:

Expected behavior

Environment:

  • OS: macOS
  • @graphql-inspector/cli: ^4.0.2
  • graphql: ^16.8.1
  • NodeJS: v18.10.0

Additional context

Here is a basic reproduction using the linked logic above:

https://www.typescriptlang.org/play?#code/MYewdgzgLgBAhgJwOYRgXhgbwFAzzACwFM4ATIhALhgG1d8GAiAeQQEsk2xqCooAHSgHohAGxDA4ogiGiUArIvmMANPTwBdbAF9s2UUVjEyFVBkzb4qAEpFQCUgB5o7MEhUwXXJAD4A3HoAZiAIMAAUoJBGJOShIIHwyBAAdMaxAJRY6jCR0LRgcAC2RB7JZQBuUgCuRBAa6IQxFMkQ-KJsUGEA5JRd6QHZaaY0BcX1GJWiNSkAViBc3X0Buti5IAbJ4khhQwgQ6XoGsEggIKQAEk17DRZWMLb2Tl5uHs++A8GhEeB5uzDxiRQqSumRwDFysBGRRKMDKyUm03GjRMCBabQ63V6-T0DBOZ0uKIgULGDQRtWShTg-DCYRcmTQPk8UFRzLYhTC6XSyTmCx6Sx0ejWGy2YTxFyu+yAA

@Urigo Urigo added the stage/1-reproduction A reproduction exists label Nov 19, 2023
@Urigo
Copy link
Collaborator

Urigo commented Nov 19, 2023

Thanks for reporting @aaronleopold !

Sorry but I'm not adding a lot here but just labeling it according to our new Contribution Guide and issue flow.

It seems already got into stage 1 thanks to your reproduction! Thank you for that!

Now in order to advance to stage 2 we'll need a failing test, do you think you are able to create one?

Thank you and sorry that this comment is not a complete solution (yet).

@aaronleopold
Copy link
Author

aaronleopold commented Nov 19, 2023

Thanks for reporting @aaronleopold !

Sorry but I'm not adding a lot here but just labeling it according to our new Contribution Guide and issue flow.

It seems already got into stage 1 thanks to your reproduction! Thank you for that!

Now in order to advance to stage 2 we'll need a failing test, do you think you are able to create one?

Thank you and sorry that this comment is not a complete solution (yet).

Hey 👋 no worries!

When you ask for a failing test, are you asking for a code change that literally adds a failing test somewhere? Sorry if that seems an obvious question haha just want to clarify. If so, I can probably find time next weekend to do so. However, my playground link has a tentative fix underneath the reproduction, so at that point if I'm already issuing a PR would it just be preferred to make the fix directly?

@Urigo
Copy link
Collaborator

Urigo commented Nov 20, 2023

Yes exactly.
Even if you already have the solution, we always try to include a test case with our solutions so we'll make sure it won't happen again.
The best case is to submit a PR with a failing test, see that it actually fails on CI and then push another commit with the fix.
Thank you so much for you help!

@AdelELHAIBA-Akeneo
Copy link

Hello, we have the same errors and after some debug we understand that it was on graphql-inspector and we found this issue created.
Any way to help on this issue to make it fixed quickly ? 😃

@aaronleopold
Copy link
Author

Any way to help on this issue to make it fixed quickly ? 😃

Sorry this fell off my radar since I didn't have time to wait for the outlined process because of a work requirement (and try not to work too much on the weekends 😅). I'll try to create the PR with the failing CI and followup with a correcting commit, but if you (@AdelELHAIBA-Akeneo) need the fix sooner my reproduction had a tentative fix you can use if you have the time to do what @Urigo outlined above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/1-reproduction A reproduction exists
Projects
None yet
Development

No branches or pull requests

3 participants