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

fix: identify classes with an Integer ID field [DHIS2-16092] #15554

Merged
merged 2 commits into from
Nov 2, 2023
+12 −3
Merged

fix: identify classes with an Integer ID field [DHIS2-16092] #15554

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4730fa8
fix: identify persistent classes with an INT id field in the cache in…
netroms Nov 1, 2023
aead875
Merge branch 'master' into DHIS2-16092
mortenoh Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions ...dation/src/main/java/org/hisp/dhis/cacheinvalidation/redis/CacheInvalidationListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import io.lettuce.core.pubsub.RedisPubSubListener;
import java.io.Serializable;
import java.lang.reflect.Field;
import java.util.Objects;
import lombok.extern.slf4j.Slf4j;
import org.hibernate.SessionFactory;
Expand All @@ -40,7 +41,6 @@
import org.hisp.dhis.dataelement.DataElement;
import org.hisp.dhis.dataset.CompleteDataSetRegistration;
import org.hisp.dhis.dataset.DataSet;
import org.hisp.dhis.datastatistics.DataStatisticsEvent;
import org.hisp.dhis.datavalue.DataValue;
import org.hisp.dhis.organisationunit.OrganisationUnit;
import org.hisp.dhis.period.Period;
Expand Down Expand Up @@ -158,9 +158,18 @@ private Serializable getEntityId(String message) throws ClassNotFoundException {
return getTrackedEntityAttributeValueId(idPart);
} else if (CompleteDataSetRegistration.class.isAssignableFrom(entityClass)) {
return getCompleteDataSetRegistrationId(idPart);
} else if (DataStatisticsEvent.class.isAssignableFrom(entityClass)) {
return Integer.parseInt(idPart);
} else {
try {
// Best effort to try to identify classes with int IDs.
Field idField = entityClass.getDeclaredField("id");
Class<?> idType = idField.getType();
if (idType == int.class) {
return Integer.parseInt(idPart);
Dismissed Show dismissed Hide dismissed
}
} catch (NoSuchFieldException e) {
// Ignore this exception, as it's expected unless it's a class with an int ID.
}
// In most cases the ID will be a long.
return Long.parseLong(idPart);
Copy link
Contributor

@david-mackessy david-mackessy Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like if there is no id field and the NoSuchFieldException is thrown that we still try parse the idPart as a long even though there is no id field. Is that what we want to try here?
Some tests for this logic would be beneficial I think, with so many conditional branches in this method.

Copy link
Contributor Author

@netroms netroms Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My thinking here is that if there is an ID field on the class it will be an Integer. If you do a search for "int id;" you will see the result is only classes that has the ID field on the actual class and don't inherit it from the superclass. So when it's not an int field, it will be a Long, mostly via the BaseIdentifiableObject. Making logic to catch every single case here is not the intention.

}
}
Expand Down
Loading