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

Simplify condition to invoke "resolveCodec" method in AnnotationRedisCodecResolver #1149

Closed
machi1990 opened this issue Oct 15, 2019 · 4 comments
Labels
type: task A general task
Milestone

Comments

@machi1990
Copy link
Contributor

While going through the AnnotationRedisCodecResolver.java class, I noticed that this condition

https://github.com/lettuce-io/lettuce-core/blob/c98108dd7fbe9bee8702e468efff4b085a976374/src/main/java/io/lettuce/core/dynamic/codec/AnnotationRedisCodecResolver.java#L73-L76 can be improved. I have a refactor in place and I am preparing a PR.

@mp911de
Copy link
Collaborator

mp911de commented Oct 15, 2019

Thanks for having a look. The code tries to find whether there is a single key(value) codec and no or up to one value (key) codecs. Simplifying the condition to keyTypes.size() == 1 || valueTypes.size() == 1 allows valueTypes to contain two items when keyTypes contains a single type. We don't have a test that that is able to fail in this case.

Something like String mixedCodecs(@Key String key1, @Key byte[] key2, @Value String value); would be able to reproduce the issue.

@mp911de mp911de added the status: waiting-for-feedback We need additional information before we can continue label Oct 15, 2019
@machi1990
Copy link
Contributor Author

Hi @mp911de thanks for the feedback, what you are saying is String mixedCodecs(@Key String key1, @Key byte[] key2, @Value String value) should throw an IllegalStateException exception?

if so I can add the test (looks like there is no test for this case) and make sure it passes.

Thanks.

@mp911de
Copy link
Collaborator

mp911de commented Oct 15, 2019

Yes, please.

@machi1990
Copy link
Contributor Author

@mp911de thanks for the inputs. Added a test commit. Let me know what you think regarding the PR.

mp911de pushed a commit that referenced this issue Oct 16, 2019
…isCodecResolver #1149

This adds a 'methodHasAtMostOneCodec' utility method to check whether a
CommandMethod has at most one codec

Original pull request: #1150.
mp911de added a commit that referenced this issue Oct 16, 2019
Add author tags. Remove final from local variables. Extract atMostOne check into method.

Original pull request: #1150.
mp911de pushed a commit that referenced this issue Oct 16, 2019
…isCodecResolver #1149

This adds a 'methodHasAtMostOneCodec' utility method to check whether a
CommandMethod has at most one codec

Original pull request: #1150.
mp911de added a commit that referenced this issue Oct 16, 2019
Add author tags. Remove final from local variables. Extract atMostOne check into method.

Original pull request: #1150.
@mp911de mp911de added type: task A general task and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 16, 2019
@mp911de mp911de added this to the 5.2.1 milestone Oct 16, 2019
@mp911de mp911de closed this as completed Oct 16, 2019
@mp911de mp911de changed the title refactor simplify condition to invoke "resolveCodec" method in AnnotationRedisCodecResolver Simplify condition to invoke "resolveCodec" method in AnnotationRedisCodecResolver Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants