alberto-art3ch commented on PR #6493:
URL: https://github.com/apache/fineract/pull/6493#issuecomment-6450260357
> Thanks for the update. The caching design matches the description, and
most of the earlier feedback is addressed. Two things remain open, plus a
rebase:
>
> 1. **The datatable error-handling change is still in the diff** (inline
comment on `DatatableWriteServiceImpl`).
> 2. **Registering a PPI datatable doesn't evict the new all-properties
cache** (inline comment on `addSurveyConfig`).
> 3. The PR now conflicts with develop (`FineractEntityAccessUtil`), so it
needs a rebase.
Thanks @adamsaghy, all three addressed and the branch is rebased on develop.
1. **Datatable error handling**: dropped. My earlier reply said it was
removed but the hunk was still in the diff, sorry for the noise.
`DatatableWriteServiceImpl#updateDatatable` is now byte-identical to develop;
the change will come as its own PR with a test.
2. **PPI datatable registration**: you are right, and the same gap exists in
the opposite direction. `deregisterDatatable` deletes the `c_configuration` row
with plain JDBC too, so after deleting a PPI datatable the configuration kept
resolving from the cached snapshot until the TTL. Both paths now call
`GlobalConfigurationRepositoryWrapper#removeFromCache()` right after the JDBC
statement. I kept the JDBC insert rather than routing through
`addSurveyConfig`: that method has no callers, creates the row without the
`value = '0'` the insert sets, and maps data-integrity errors differently, so
reusing it would have changed behaviour beyond the eviction.
Tests:
- Unit (`DatatableWriteServiceImplTest`): registering with `CATEGORY_PPI`
evicts, registering with `CATEGORY_DEFAULT` does not touch the cache,
deregistering evicts.
- Integration (`GlobalConfigurationTest`): switches to SINGLE_NODE,
caches the snapshot with a read, re-registers a datatable as PPI and asserts
`GET /configurations/name/{dt}` finds it, then deletes the datatable and
asserts the lookup is 404. NO_CACHE is restored in a `finally`. Without the
evictions the first assertion fails with 404 and the second returns the stale
configuration.
3. **Rebase**: done. The conflict in `FineractEntityAccessUtil` was develop
replacing the SQL in-clause with `filterVisibleToOffice_ifGlobalConfigEnabled`;
resolved by keeping develop's method with the wrapper's new
`GlobalConfigurationPropertyData` return type. The new
`ConfigurationDomainServiceJpaTest` from FINERACT-2940 stubbed the wrapper with
the entity, adjusted to the data type.
--
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]
