FINERACT-2455: enforce NOT NULL on WC submitted_on_date columns

[mariiaKraievska]

Description

Describe the changes made and why they were made. (Ignore if these details are present on the associated Apache Fineract JIRA ticket.)

Checklist

Please make sure these boxes are checked before submitting your pull request - thanks!

• Write the commit message as per our guidelines
• Acknowledge that we will not review PRs that are not passing the build ("green") - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
• Create/update unit or integration tests for verifying the changes made.
• Follow our coding conventions.
• Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
• This PR must not be a "code dump". Large changes can be made in a branch, with assistance. Ask for help on the developer mailing list.
• If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.

Your assigned reviewer(s) will follow our guidelines for code reviews.

[galovics]

The changelog itself is well-formed - separate part file, backfill before the constraint, MARK_RAN preconditions, matching column data types. My concern is with deploy sequencing rather than the SQL.

#6299 (which added the nullable submitted_on_date column) is itself unreleased. If both PRs ship in the same release, then during a rolling upgrade the "old" instances are running code that never writes submitted_on_date at all - it doesn't exist in their entity mapping. Every insert from those instances hits the NOT NULL constraint the moment this migration has run on the new schema, and the whole write path 500s until the rollout completes. The re-backfill changeset here only protects against N-1 writers that do know about the column but raced the earlier backfill - it can't protect against N-1 writers that don't know about the column at all.

On top of that, the backfill and the addNotNullConstraint are two separate changesets/transactions, so there's a window between them where a concurrent write can still insert NULL and abort the whole migration (rather than just leaving a bad row) once the constraint changeset runs.

Two ways to fix this:

1. If #6299 and this PR are meant to ship together, add a DEFAULT value (e.g. CURRENT_DATE) before enforcing NOT NULL, so N-1 writers that omit the column entirely still get a valid value rather than an error. And merge the backfill + constraint into one changeset per table so the whole thing is atomic.
2. If they're meant to ship in separate releases, this PR should wait for the next one, and the comment on the re-backfill changeset should be corrected to explain it's belt-and-braces (for the narrower "column exists but wasn't written" case) rather than implying it closes the full rolling-deploy window.

The in-repo precedent for exactly this situation (parts/0016_configurable_attributes_not_null.xml) adds a default value for this reason and explicitly says so in its comment - worth following that pattern here.

Recommendation: CHANGES_REQUESTED

[mariiaKraievska]

The changelog itself is well-formed - separate part file, backfill before the constraint, MARK_RAN preconditions, matching column data types. My concern is with deploy sequencing rather than the SQL.

#6299 (which added the nullable submitted_on_date column) is itself unreleased. If both PRs ship in the same release, then during a rolling upgrade the "old" instances are running code that never writes submitted_on_date at all - it doesn't exist in their entity mapping. Every insert from those instances hits the NOT NULL constraint the moment this migration has run on the new schema, and the whole write path 500s until the rollout completes. The re-backfill changeset here only protects against N-1 writers that do know about the column but raced the earlier backfill - it can't protect against N-1 writers that don't know about the column at all.

On top of that, the backfill and the addNotNullConstraint are two separate changesets/transactions, so there's a window between them where a concurrent write can still insert NULL and abort the whole migration (rather than just leaving a bad row) once the constraint changeset runs.

Two ways to fix this:

1. If FINERACT-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration #6299 and this PR are meant to ship together, add a DEFAULT value (e.g. CURRENT_DATE) before enforcing NOT NULL, so N-1 writers that omit the column entirely still get a valid value rather than an error. And merge the backfill + constraint into one changeset per table so the whole thing is atomic.
2. If they're meant to ship in separate releases, this PR should wait for the next one, and the comment on the re-backfill changeset should be corrected to explain it's belt-and-braces (for the narrower "column exists but wasn't written" case) rather than implying it closes the full rolling-deploy window.

The in-repo precedent for exactly this situation (parts/0016_configurable_attributes_not_null.xml) adds a default value for this reason and explicitly says so in its comment - worth following that pattern here.

Recommendation: CHANGES_REQUESTED

@galovics Thank you for your review.
0074 now does DEFAULT → backfill → NOT NULL in one changeset per table; verified on MariaDB and PostgreSQL that N-1 omit-column inserts succeed. Please take another look

[adamsaghy]

The changelog itself is well-formed - separate part file, backfill before the constraint, MARK_RAN preconditions, matching column data types. My concern is with deploy sequencing rather than the SQL.

#6299 (which added the nullable submitted_on_date column) is itself unreleased. If both PRs ship in the same release, then during a rolling upgrade the "old" instances are running code that never writes submitted_on_date at all - it doesn't exist in their entity mapping. Every insert from those instances hits the NOT NULL constraint the moment this migration has run on the new schema, and the whole write path 500s until the rollout completes. The re-backfill changeset here only protects against N-1 writers that do know about the column but raced the earlier backfill - it can't protect against N-1 writers that don't know about the column at all.

On top of that, the backfill and the addNotNullConstraint are two separate changesets/transactions, so there's a window between them where a concurrent write can still insert NULL and abort the whole migration (rather than just leaving a bad row) once the constraint changeset runs.

Two ways to fix this:

1. If FINERACT-2455: WC - created and submitted date of all features supported in WCP should follow business date or system date based on configuration #6299 and this PR are meant to ship together, add a DEFAULT value (e.g. CURRENT_DATE) before enforcing NOT NULL, so N-1 writers that omit the column entirely still get a valid value rather than an error. And merge the backfill + constraint into one changeset per table so the whole thing is atomic.
2. If they're meant to ship in separate releases, this PR should wait for the next one, and the comment on the re-backfill changeset should be corrected to explain it's belt-and-braces (for the narrower "column exists but wasn't written" case) rather than implying it closes the full rolling-deploy window.

The in-repo precedent for exactly this situation (parts/0016_configurable_attributes_not_null.xml) adds a default value for this reason and explicitly says so in its comment - worth following that pattern here.

Recommendation: CHANGES_REQUESTED

While Arnold's concern is valid, the source for backfilling the date seems incorrect to me. I would recommend using the audit date time (date party only) instead of backfilling with current date.