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

Add optional query string usage in resolvers #453

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DanailPenev
Copy link

Summary

  • Add query-string library as a dependency
  • Add an optional parameter useQueryString to RequestOptions which allows callers to use the query string library when generating the resolvers
  • This enables use cases such as sending array query parameters like ?x=a&x=b&x=c compared to the existing behaviour of ?x[0]=a&x[1]=b&x[2]=c
  • The query string library does not behave nicely with nested objects, such as the test case in packages/openapi-to-graphql/test/example_api6.test.ts so if we want to fully migrate to using it, we might have to add some custom logic

Motivation

Testing

  • Add a unit test that verifies the behaviour with or without the new optional parameter
  • Verify it's working as intended in a private project
- Add an optional parameter `useQueryString` to `RequestOptions` which allows callers to use the query string library when generating the resolvers
- Add branching logic that uses either query-string or the previous custom logic

Signed-off-by: Dani Penev <danailnpenev@gmail.com>
@thomsonbw
Copy link

Thanks, Danail. We have a private fix too, but instead of a boolean option we added a function-valued option "queryStringify" so the application could implement whatever strategy it wanted. I was reluctant to propose it here though because I didn't think a global setting is the correct answer when graphql is aggregating REST services from several sources. Using Openapi's style/format/explode properties seems better, since they allow different services to use different strategies, but I'm not sure they cover all the array formats we are likely to run into. What is your experience?

@DanailPenev
Copy link
Author

@thomsonbw, I agree with you completely that the change I am proposing is not a perfect solution. Ideally, the parameter serialization should be driven by the OpenAPI spec to conform to the different array query parameter formats there are with an optional override if something is going wrong.

I think the boolean option is a good middle ground right now because it exposes a straightforward solution to others who might be struggling with the same issue. It also restores the previously existing functionality that was removed after 2.4.0 when the request library was swapped for cross-fetch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants