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

feat: Remove usage of Guava #206

Merged
merged 2 commits into from
Jun 8, 2023

Conversation

ctasada
Copy link
Collaborator

@ctasada ctasada commented Jun 5, 2023

The Guava library is really convenient, but over time, most of its features became part
of the vanilla Java.

This PR removes the usage of Guava, replacing it by Java equivalents.

@ctasada ctasada force-pushed the ctasada/remove-guava branch 2 times, most recently from 858c196 to 78d3b96 Compare June 6, 2023 06:48
@tvahrst
Copy link
Contributor

tvahrst commented Jun 6, 2023

Concerning MessageHelperTest: there is no garantuee, which of message2,message3 won the deduplication, so I suggest this assertion:

        Map<String, List<Message>> oneOfMap = (Map<String, List<Message>>) asObject;
        assertThat(oneOfMap).hasSize(1);
        List<Message> deduplicatedMessageList = oneOfMap.get("oneOf");
        // we do not have any guarantee wether message2 or message3 won the deduplication.
        assertThat(deduplicatedMessageList)
                .hasSize(2)
                .contains(message1)
                .containsAnyOf(message2, message3);

BTW: MessageHelper#toMessageObjectOrComposition expects a Set of Messages, but Message does not implement equals/hashcode. So the Set Sematics are not used and the Set is defacto a List of Messages. Internally, duplicates are removed via a special stream using a messageSupplier which in turn uses a TreeSet which in turn is configured with a Comparator which compares the name of the message.
If the message name was the 'unique' id of a message, this could be easier achieved by simply implementing equals/hashcode in the Message class.

@tvahrst
Copy link
Contributor

tvahrst commented Jun 6, 2023

Concerning Async*AnnotationScannerTest: The order of entries in map is not garantueed, so it should be:

 assertThat(actualChannels)
                .containsExactlyInAnyOrderEntriesOf(
                        Map.of(
                                "test-channel-1", expectedChannel1,
                                "test-channel-2", expectedChannel2));

instead of containsExactlyEntriesOf

@ctasada ctasada force-pushed the ctasada/remove-guava branch from 78d3b96 to d174127 Compare June 6, 2023 14:30
@ctasada
Copy link
Collaborator Author

ctasada commented Jun 6, 2023

@tvahrst Thanks for the suggestions. They work like a charm 😄

Regarding the equals comment. Message implements Lombok's @Data annotation, which adds the @EqualsAndHashCode. So the generated Message class is implementing equals/hashcode.

Is the MessageHelper the one that uses a Message::getName comparator to remove the Message duplications, based on the name.

I would say that this is the point that should be discussed. Are 2 Messages with same name, but different description equals? If that's the case, then we could probably modify the equals implementation to ignore the description, providing a much more consistent (and simple) behaviour across the library.

@timonback @tvahrst what do you think?

@timonback
Copy link
Member

@ctasada That is a good question, whether a message is equal or not.

At this point, Springwolf only uses the name of the message - as the name is used in the ui through the schema name. As soon as we have methods which get detected multiple times (i.e. KafkaListener and AsyncSubscriber annotation together), we find the same message payload class twice, but only want to show it once - first one wins.
(There are other use-cases, where we do not want to rely only on the class name. For example, when we have the class name in different packages -> limitation)

I am fine to continue to use only the name for checking the equality of a message.

When looking at the spec, there is the option to use Message#messageId for a unique name.
We can improve further (in the future/other PR) - as we might need to adapt springwolf-ui as well.

Feel free to look into it.

@ctasada
Copy link
Collaborator Author

ctasada commented Jun 7, 2023

@timonback I think this PR is ready for merge. I don't think this PR introduces any behavioral change.

The suggested change regarding Message comparison I think should be attacked in it's own PR. I can take a look later

Carlos Tasada added 2 commits June 8, 2023 08:31
The Guava library is really convenient, but over time, most of its features became part
of the vanilla Java.

This PR removes the usage of Guava, replacing it by Java equivalents.
The 'javax.annotation.Nullable' was, in fact, imported from 'com.google.code.findbugs:jsr305' which was a transitive Guava dependency.

We replace this annotation by the SpringFramework equivalent.
@ctasada ctasada force-pushed the ctasada/remove-guava branch from d174127 to 7aba4db Compare June 8, 2023 06:31
Copy link
Member

@timonback timonback left a comment

Choose a reason for hiding this comment

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

Thank you very much to go through all these occurrences and replacing them with the latest Java features in order to remove the guava dependency!

@timonback timonback merged commit 7b461fa into springwolf:master Jun 8, 2023
@ctasada ctasada deleted the ctasada/remove-guava branch December 13, 2023 17:05
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.

3 participants