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 StatusCodeFunc for HTTP Transport implementations #965

Closed
wants to merge 1 commit into from

Conversation

vvakame
Copy link
Collaborator

@vvakame vvakame commented Dec 24, 2019

revise #948
add method that modify HTTP status code

This PR is works, but maybe not the best.

I want to add StatusCode(ctx context.Context, resp *graphql.Response) int but graphql.Transport doesn't limited to HTTP handling.

so, how about below API design?

type TransportConfigurator interface {
  AttachTransports (ctx context, transports []graphql.Transport)
}

and

func (a AutomaticPersistedQuery) AttachTransports (ctx context, transports []graphql.Transport) {
  for _, transport := range transports {
    httpTransport, ok := transport.(interface { HookStatusCode(f func(ctx context.Context, resp *graphql.Response) int) })
    if !ok { continue }
    httpTransport.HookStatusCode(func(ctx context.Context, resp *graphql.Response) int {
      if len(resp.Errors) == 1 && resp.Errors[0].Message == "PersistedQueryNotFound" {
        return http.StatusOK
      }
      return 0
    })
  }
}

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@vvakame vvakame requested a review from vektah December 24, 2019 12:14
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 63.943% when pulling 5838cbc on feat-apq-status into f869f5a on master.

@vektah
Copy link
Collaborator

vektah commented Jan 7, 2020

Alternative #978, what do you think?

Even if we dont take it we should keep the integration tests.

@vektah vektah closed this in #978 Jan 15, 2020
@vvakame vvakame deleted the feat-apq-status branch January 15, 2020 05:57
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.

3 participants