adamsaghy commented on code in PR #6493:
URL: https://github.com/apache/fineract/pull/6493#discussion_r4383750331
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/DatatableWriteServiceImpl.java:
##########
@@ -490,6 +488,11 @@ public void updateDatatable(final String datatableName,
final JsonCommand comman
throw new
PlatformServiceUnavailableException("error.msg.datatable.column.update.not.allowed",
"One of the data table columns contains null
values", e);
}
+ // Every other failure must surface. Swallowing it
reported success to the client while, on
+ // PostgreSQL, the transaction was already aborted: the
next statement of the command pipeline
+ // then failed with an exception that depended on what ran
first (a wrapped query error mapped to
+ // 403, a raw EclipseLink error from the audit merge to
500).
+ throw e;
Review Comment:
The PR description lists the datatable DDL error mapping as out of scope
(separate PR), and the reply to the earlier review says it was removed. It is
still here: the catch is now `RuntimeException` and ends with `throw e;`, where
develop swallowed the exception.
This is client-visible. A `PUT /datatables/{name}` whose ALTER/index step
fails for any other reason used to return success and now returns an error.
That contradicts "No client-visible behaviour change", and there's no test for
it. Please drop it from this PR and send it separately, as described.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/service/GlobalConfigurationWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -84,6 +84,8 @@ public void addSurveyConfig(final String name) {
try {
final GlobalConfigurationProperty ppi =
GlobalConfigurationProperty.newSurveyConfiguration(name);
this.repository.save(ppi);
+ // The cached snapshot of all properties must not outlive a newly
created one.
+ this.repository.removeFromCache();
Review Comment:
This eviction is right, but there's a second path that creates a
configuration row and skips it: `DatatableWriteServiceImpl#registerDatatable`
inserts into `c_configuration` with plain JDBC when the category is PPI, and
never calls `removeFromCache()`.
In SINGLE_NODE mode, once any config read has cached `<tenant>::all`,
registering a PPI datatable leaves `GET /configurations/name/{dt}` and `PUT
/configurations/name/{dt}` returning 404 until the cache entry expires. On
develop the per-name cache never cached a miss, so this didn't happen. Please
evict there too (or route it through `addSurveyConfig`), ideally with a test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
