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

Stream transaction support in the Collection and Graph APIs #403

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

tjoubert
Copy link
Contributor

@tjoubert tjoubert commented Jul 30, 2022

Solves issue #328

  • Added ApiHeaderProperties allowing users to pass any header (as long as it is supported) to a method. This will be useful for situations in the future where the API adds new headers and we have not had the time to add named properties/parameters for these new headers in the driver.
  • Added stream transaction support in the Collection and Graph APIs

@rossmills99
Copy link
Collaborator

Nice idea to add support for additional headers. I realise it looks quite trivial but wonder if we should consider having one or mote simple unit tests for ToWebHeaderCollection method?

Copy link
Collaborator

@rossmills99 rossmills99 left a comment

Choose a reason for hiding this comment

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

Looks good - I made a comment that you could consider adding a unit test for the new ToWebHeaderCollection method but I don't think it's a blocker.

@tjoubert tjoubert added the enhancement New feature or request label Aug 2, 2022
@DiscoPYF DiscoPYF linked an issue Aug 2, 2022 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.

Very nice! Approved 👍
I agree with Ross' comment that it would be good to add tests to cover any new methods we add, but not a blocker.

With the new ApiHeaderProperties class you created, we could open a follow-up enhancement to add it to every API methods to support general headers, such as x-arango-async.

Another tought: I wonder if we should create dedicated classes e.g. GraphHeaderProperties and CollectionHeaderProperties to prevent breaking changes in future if headers specific to those APIs are introduced and we then need to define those classes. Not a blocker.

Let me know what you think. Happy to see this merged in any case. 🚀

@tjoubert tjoubert merged commit cb032dc into master Aug 8, 2022
@tjoubert tjoubert deleted the feature-3.8/DE-249-stream-transaction-support branch August 8, 2022 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Stream Transactions in Collection and Graph APIs
3 participants