Skip to content

chore: DataSet annotation mapping - #25014

Merged
vietnguyen merged 10 commits into
masterfrom
hbm-jpa-data-set
Oct 7, 2026
Merged

vietnguyen merged 10 commits into
masterfrom
hbm-jpa-data-set

Conversation

@vietnguyen

@vietnguyen vietnguyen commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

https://dhis2.atlassian.net/browse/DHIS2-20117

Migrates DataSet and DataInputPeriod from HBM XML to JPA annotations.

  • @JsonSerialize now targets IdentifiableObject instead of BaseIdentifiableObject/BaseNameableObject.
  • AbstractHibernateListener: audit re-fetches through the proxy only for singular embedded properties or unloaded collections, so embedded collections aren't lost.
  • Tests: extended DataSetStoreTest; added an import test for DataSetElements without back-references.

@vietnguyen vietnguyen added the run-api-analytics-tests Enables analytics e2e tests label Sep 2, 2026
vietnguyen and others added 8 commits September 8, 2026 04:56
…g orphan-removal exception

CI reported HibernateException "A collection with cascade=all-delete-orphan was no
longer referenced by the owning entity instance" across DataSet.dataSetElements and
DataSet.dataInputPeriods, breaking basic create/patch flows.

Two compounding root causes:

1. DataInputPeriod was never migrated to JPA - it was still purely HBM-mapped
   (DataInputPeriod.hbm.xml) while DataSet.dataInputPeriods already declared a JPA
   @onetomany @joincolumn targeting it. A JPA-owned @onetomany requires its target to
   be a genuine @entity; the DB schema already had the datasetid column ready, this
   was a pure Java-mapping gap. Converted DataInputPeriod to @entity and removed its
   HBM file.

2. setDataSetElements/setDataInputPeriods/setCompulsoryDataElementOperands had been
   changed to clear-and-repopulate the existing collection instead of a plain field
   assignment. Hibernate's own PojoEntityTuplizer calls these setters internally via
   reflection during session.save()/flush() to install its tracked PersistentCollection
   wrapper - a setter that doesn't accept the new reference leaves the entity's field
   pointing at the old collection while Hibernate believes its wrapper was installed,
   throwing the orphan-removal exception on the next flush, even for a brand-new
   entity with an empty collection. Reverted to plain assignment; the dataSetElements
   back-reference wiring (needed since it's the mappedBy/inverse side) now happens in
   a separate loop alongside the assignment, not instead of it.

All 4 originally-failing tests pass (AbstractCrudControllerTest.testPatchRemoveById,
testPatchSharingUserGroups, DataSetControllerTest.responseTypeTest,
MetadataImportExportControllerTest.removeDataSetIndicatorTest). Full dhis-test-web-api
H2-backed suite passes; remaining Testcontainers/Postgres-backed test failures in that
run are a pre-existing local Docker-unavailability issue, unrelated to this change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
private int id;

/** Period data must belong to */
@ManyToOne(fetch = FetchType.LAZY)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure LAZY is better here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can change to EAGER, so when Hibernate loads that collection, it builds the collection query from the mapping and outer-joins an EAGER many-to-one. So you get the input periods and their Periods in one SQL statement, instead of one query per proxy. This could improve query time a bit.

@sonarqubecloud

sonarqubecloud Bot commented Oct 7, 2026

Copy link
Copy Markdown

@vietnguyen
vietnguyen merged commit fff6a4c into master Oct 7, 2026
25 checks passed
@vietnguyen
vietnguyen deleted the hbm-jpa-data-set branch October 7, 2026 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-api-analytics-tests Enables analytics e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants