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.

Sign up for GitHub

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

Jump to bottom

ReactiveVaultTemplate for the key-value backend version 2 #807

Closed
wants to merge 12 commits into from
Closed

ReactiveVaultTemplate for the key-value backend version 2 #807

wants to merge 12 commits into from

Conversation

tweiand
Copy link
Contributor

@tweiand tweiand commented Jul 20, 2023 •
edited
Loading

What is done:

  • Add version 2 (by porting blocking code APIs)
  • Unit tests for all additional classes
  • Maybe some code deduplication and cleanup
  • A second round of editing java docs
  • Using WebClient to do all object conversion
  • Created data object to represent the v2 nested data

Questions I have:

  • Is author attribution correct since this can be considered a port of the block code?
  • Overall, what do you think about the change?

What I am looking at:

  • Java docs
  • Code deduplication and static analysis
  • Cleaning up the diff

I am available for video chat.

@tweiand tweiand mentioned this pull request Jul 20, 2023
Closed
@mp911de mp911de self-assigned this Aug 10, 2023
@mp911de mp911de added this to the 3.1.0-M1 milestone Aug 10, 2023
mp911de
mp911de requested changes Aug 10, 2023
View reviewed changes
Copy link
Member

@mp911de mp911de left a comment

Choose a reason for hiding this comment

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

I had a first look. The structure looks pretty much like the imperative support. There are a few deviations that we should discuss, I left a few comments.

spring-vault-core/pom.xml
@@ -324,4 +324,89 @@
</dependency>
</dependencies>

<profiles>
Copy link
Member

Choose a reason for hiding this comment

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

We do not use Sonar so please remove the additional profile.

...-vault-core/src/main/java/org/springframework/vault/core/ReactiveVaultKeyValue2Template.java Outdated Show resolved Hide resolved
...g-vault-core/src/main/java/org/springframework/vault/core/ReactiveVaultKeyValueAccessor.java Outdated Show resolved Hide resolved
...core/src/main/java/org/springframework/vault/core/ReactiveVaultKeyValueMetadataTemplate.java Outdated Show resolved Hide resolved
...ng-vault-core/src/main/java/org/springframework/vault/support/VaultResponseDataVersion2.java
import org.springframework.lang.Nullable;

@JsonIgnoreProperties(ignoreUnknown = true)
public class VaultResponseDataVersion2<T> {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason why we require an additional response wrapper and why we cannot follow the imperative typing approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal was to force WebClient to do all JSON marshaling. My intent was to use any added Jackson deserialization to the DI'ed WebClient.Builder.

To do what imperative did - do you know of a way to get the default Jackson deserialization configuration from WebClient?

...va/org/springframework/vault/core/ReactiveVaultKeyValueMetadataTemplateIntegrationTests.java Outdated Show resolved Hide resolved
spring-vault-core/src/main/java/org/springframework/vault/core/ReactiveKeyValueHelper.java Outdated Show resolved Hide resolved
@tweiand
Copy link
Contributor Author

tweiand commented Aug 25, 2023

@mp911de - I had a bad git config when doing most of the work on this. After we finish review, would it be possible for me to re-make the branch so that the commits look correct?

@mp911de
Copy link
Member

mp911de commented Aug 26, 2023 via email

@tweiand
Copy link
Contributor Author

tweiand commented Aug 28, 2023

Force pushed the branch with the correct email for me. I have 2 questions buried in the comments. Thanks for the help!

@mp911de mp911de changed the title Draft: ReactiveVaultTemplate for the key-value backend version 2 ReactiveVaultTemplate for the key-value backend version 2 Sep 25, 2023
@mp911de mp911de added the type: enhancement A general enhancement label Sep 25, 2023
@mp911de mp911de linked an issue Sep 25, 2023 that may be closed by this pull request
ReactiveVaultTemplate for the key-value backend version 2 #576
Closed
mp911de pushed a commit that referenced this pull request Sep 25, 2023
@tweiand @mp911de
ReactiveVaultTemplate for the key-value backend version 2.
c303f55 
Closes gh-576
Original pull request: gh-807
mp911de added a commit that referenced this pull request Sep 25, 2023
@mp911de
Polishing.
a48322f 
Align Object Mapping with imperative approach. Refactor read methods to improve empty handling without the use of exceptions. Revert code reorganization. Improve encapsulation.

Reformat code. Simplify tests.

See gh-576
Original pull request: gh-807
@mp911de
Copy link
Member

mp911de commented Sep 25, 2023

Thank you for your contribution. That's merged and polished now. I took care of all license headers, author and since-tags.

@mp911de mp911de closed this Sep 25, 2023
mp911de added a commit that referenced this pull request Sep 25, 2023
@mp911de
Polishing.
c1406d7 
Align Object Mapping with imperative approach. Refactor read methods to improve empty handling without the use of exceptions. Revert code reorganization. Improve encapsulation.

Reformat code. Simplify tests.

See gh-576
Original pull request: gh-807
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de mp911de requested changes

Assignees

@mp911de mp911de

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
3.1.0-M1
Development

Successfully merging this pull request may close these issues.

ReactiveVaultTemplate for the key-value backend version 2
2 participants
@tweiand @mp911de