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

Provide response headers to all API methods #469

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

tjoubert
Copy link
Contributor

  1. Added ApiClientBase.GetResponseAsync() to extract the response body and headers to an object of type ApiResponse<T> where T is the response type.
  2. Amended ApiClientBase.GetApiErrorExceptionAsync() to extract header values on error and assign that to ApiErrorException.Headers (a new dictionary property).
  3. As an example, I changed ViewsApiClient.GetAllViewsAsync() to return an object of type ApiResponse<GetAllViewsResponse> instead of GetAllViewsResponse as it was before. The ApiResponse has a Headers dictionary property and will also contain any other data that we want to pass for all responses in the future.
  4. Based on our discussions on this PR, the changes in ViewsApiClient.GetAllViewsAsync() will be applied to all other API client methods. This is a major breaking change because we are changing the return values of all methods.
    The intent is to release this as part of v2.0.0 in February 2023.

@tjoubert tjoubert added enhancement New feature or request breaking change Identify issues that contain a breaking change for existing consumer code labels Jan 26, 2023
@tjoubert tjoubert self-assigned this Jan 26, 2023
@tjoubert tjoubert linked an issue Jan 26, 2023 that may be closed by this pull request
Copy link
Collaborator

@DiscoPYF DiscoPYF left a comment

Choose a reason for hiding this comment

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

This PR contains two related but different changesets:

  1. Allow specifying request headers for all API endpoints
  2. A proposed solution to expose response headers by standardizing the return type of all API endpoints

Can you split them into their own PR? It will be much easier to review and update.

For example, the request header changes are much simpler and may be merged faster, wheareas the response changes may have more discussions/questions.

@tjoubert
Copy link
Contributor Author

@DiscoPYF , this PR is the response-related aspect of the feature only. Please the PR for request-related stuff first: #467

@DiscoPYF
Copy link
Collaborator

Got it 👍

This pull request appears to be based on the commits from #467 but I'm not sure that's really needed. Anyway, I'll review that other one first then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Identify issues that contain a breaking change for existing consumer code enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide ArangoDB response headers to all API methods
2 participants