style: Fix some more sonar issues with service #1134
Conversation
| .setDescription( | ||
| manifestBom.getMetadata() | ||
| .getComponent() | ||
| .getDescription() | ||
| .replace(buildManifest.getRootPurl(), rebuiltPurl)); |
There was a problem hiding this comment.
@vibe13 I think this should fix the issue where description wasn't actually being changed. But, there appears to be no test for this as the tests were unaffected either way.
| @@ -211,7 +211,7 @@ protected boolean isFinished(TaskRun taskRun) { | |||
| protected Boolean isSuccessful(TaskRun taskRun) { | |||
| if (!isFinished(taskRun)) { | |||
| log.trace("TaskRun '{}' still in progress", taskRun.getMetadata().getName()); | |||
| return null; | |||
| return null; // FIXME: This is not really binary, but trinary state | |||
There was a problem hiding this comment.
Sonar is complaining about all these comparisons that do not use a primitive type, so I changed the comparisons to avoid the potential of null. Maybe this should use some sort of status enum instead.
| @@ -108,7 +106,6 @@ public <T> Predicate createPredicate( | |||
| } | |||
|
|
|||
| private String preprocessLikeOperatorArgument(String argument) { | |||
| return argument.replaceAll("_", Matcher.quoteReplacement(ESCAPE_CHAR + "_")) | |||
| .replaceAll("\\" + WILDCARD_CHAR, "%"); | |||
| return argument.replace("_", ESCAPE_CHAR + "_").replaceAll("\\" + WILDCARD_CHAR, "%"); | |||
There was a problem hiding this comment.
The first one is not a regex. Does the second one need to be a regex? Is it replacing everything including and after a '\' with '%'?.
dwalluck
force-pushed
the
sonar-service
branch
7 times, most recently
from
2a76d47 to
70ab6b1
Compare
| .sorted( | ||
| Comparator.comparingInt(String::length) | ||
| .reversed() // longest first | ||
| .thenComparing(Comparator.naturalOrder())) | ||
| .collect(Collectors.toCollection(LinkedHashSet::new)); | ||
| return Collections.unmodifiableSet(set); |
There was a problem hiding this comment.
I fixed this to use LinkedHashSet so that it's actually sorted.
This change also affects the test (that actually test the incorrect ordering).
I added the .thenComparing(Comparator.naturalOrder())) to handle strings of the same length so that we get a predictable ordering when the lengths are identical.
dwalluck
force-pushed
the
sonar-service
branch
8 times, most recently
from
8a407ae to
5664883
Compare
| @Override | ||
| public final boolean equals(Object o) { | ||
| if (this == o) { | ||
| return true; | ||
| } | ||
|
|
||
| if (o == null) { | ||
| return false; | ||
| } | ||
|
|
||
| Class<?> oEffectiveClass = o instanceof HibernateProxy | ||
| ? ((HibernateProxy) o).getHibernateLazyInitializer().getPersistentClass() | ||
| : o.getClass(); | ||
| Class<?> thisEffectiveClass = this instanceof HibernateProxy | ||
| ? ((HibernateProxy) this).getHibernateLazyInitializer().getPersistentClass() | ||
| : this.getClass(); | ||
|
|
||
| if (thisEffectiveClass != oEffectiveClass) { | ||
| return false; | ||
| } | ||
|
|
||
| RequestEvent requestEvent = (RequestEvent) o; | ||
| return Objects.equals(id, requestEvent.id); | ||
| } | ||
|
|
||
| @Override | ||
| public final int hashCode() { | ||
| return (this instanceof HibernateProxy) | ||
| ? ((HibernateProxy) this).getHibernateLazyInitializer().getPersistentClass().hashCode() | ||
| : getClass().hashCode(); | ||
| } |
There was a problem hiding this comment.
JPA Buddy says to not use @Data and @EqualsAndHashCode with JPA entities.
https://jpa-buddy.com/blog/lombok-and-jpa-what-may-go-wrong/
dwalluck
force-pushed
the
sonar-service
branch
5 times, most recently
from
dca1a72 to
f72aa88
Compare
dwalluck
force-pushed
the
sonar-service
branch
from
f72aa88 to
469e293
Compare
| @@ -147,4 +149,35 @@ public RequestEvent save() { | |||
| return this; | |||
| } | |||
|
|
|||
| @Override | |||
| public final boolean equals(Object o) { | |||
There was a problem hiding this comment.
@vibe13 I saw @dwalluck mentioning https://jpa-buddy.com/blog/lombok-and-jpa-what-may-go-wrong/, but can you maybe review this part of the change since you have more experience with it?
There was a problem hiding this comment.
I think that for database objects with a key (id), it makes sense to rely on the key alone for identity regardless of whether using hibernate. This is what we do with the Koji/Brew objects https://github.com/release-engineering/kojiji/blob/master/src/main/java/com/redhat/red/build/koji/model/xmlrpc/KojiBuildInfo.java#L473-L494.
dwalluck
force-pushed
the
sonar-service
branch
from
469e293 to
ee0c7bc
Compare
No description provided.