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 Httpheader argument in executeAndExtractJsonPathAsObject #372

Conversation

samhon
Copy link
Contributor

@samhon samhon commented May 28, 2021

Pull request checklist

  • Please read our contributor guide
  • Consider creating a discussion on the discussion forum
    first
  • Make sure the PR doesn't introduce backward compatibility issues
  • Make sure to have sufficient test cases

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Other (please describe):

Changes in this PR

We have HttpHeaders in executeAndExtractJsonPath, but doesn't in executeAndExtractJsonPathAsObject. Therefore, make executeAndExtractJsonPathAsObject take the HttpHeaders in the arguments.

Describe the new behavior from this PR, and why it's needed
Issue #

This is helpful when we writing the Unit Test with header values. One example is the auth token in the header. Without this PR, we need to do something like

HttpHeaders httpHeaders = new HttpHeaders();
    httpHeaders.add("Authorization", formatAuthorizationToken(loginUser.getToken()));

    Object executorResult =
        dgsQueryExecutor.executeAndExtractJsonPath(
            graphQLQueryRequest.serialize(), "data.login", httpHeaders);

    LoginUser registerUser = objectMapper.convertValue(executorResult, LoginUser.class);

With this PR, we can do it in just one line

HttpHeaders httpHeaders = new HttpHeaders();
    httpHeaders.add("Authorization", formatAuthorizationToken(loginUser.getToken()));

    LoginUser registerUser = dgsQueryExecutor.executeAndExtractJsonPathAsObject(
            graphQLQueryRequest.serialize(), "data.login", httpHeaders, LoginUser.class);

Alternatives considered

Describe alternative implementation you have considered
No

Copy link
Contributor

@berngp berngp left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I have a minor style request regarding the position of the header argument.

* @see <a href="https://github.com/json-path/JsonPath">JsonPath syntax docs</a>
* @see <a href="https://graphql.org/learn/queries/#variables">Query Variables</a>
*/
<T> T executeAndExtractJsonPathAsObject(String query, String jsonPath, HttpHeaders headers, Map<String, Object> variables, Class<T> clazz);
Copy link
Contributor

Choose a reason for hiding this comment

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

The position of the header argument is not consistent with other methods in the class. Could we add it as the last argument please.

I consider this here as an anomaly in the style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I addressed the comments and add some unit tests. Please take another look. Thanks.

@berngp berngp merged commit 1a64ff0 into Netflix:master Jun 4, 2021
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.

None yet

2 participants