search

medic / cht-user-management

GNU Affero General Public License v3.0
3 stars 1 forks source link

Foundations for Warnings System #161

Open kennsippell opened 2 months ago

kennsippell commented 2 months ago

Property Values

Currently, a staged place stores only the "original values" which come from the user. When code wants the corresponding "formatted value" for that propertly, it uses the validation library to format it on demand. When importing large CSV lists of CHUs/CHPs (like the Narok dataset), this results in >300,000 calls of Validation.formatSingle. This isn't that slow; but it slowed down a lot when we started autogenerating properties https://github.com/medic/cht-user-management/issues/47.

The work happening in #20 requires more formatted of property data. Although the performance gains from this change aren't huge (below), I view these changes as a required foundation for that work lest things get even slower.

This change therefore validates and formats the inputs once. The result is persisted as an IPropertyValue which contains .formatted and .original values. This adds complexity in the place datamodel, but simplifies upstream classes like RemotePlaceResolver.

The interface to the Validation library has changed significantly. New interface is to create a new ValidatedPropertyValue().

Remote Place Caching

Currently, the ChtApi.getPlacesWithType downloads all documents of a particular type and caches a subset of the data (id, name, lineage) as a RemotePlace. The warning system is going to need more data about these places so we can check for uniqueness (eg. unique facility codes). The doc's name information should also now be stored as "Property Value" and so this resulted in a reduced role of ChtApi (just fetches docs now), an increased role for RemotePlaceCache (it maps the doc down to a reduced set for caching), and a bit more complexity in Config.

Performance Measures

For Narok upload performance set (1640 CHPs) Before: 1170ms After: 601ms

Also includes

Overall, I think this is a fairly risky change because it touches every scenario we support and so we should test it well.

kennsippell commented 2 months ago

Hey @freddieptf - This is a big PR but it was born out of my attempt to split up #20 into smaller PRs. Let me know if you think I should split it up even more for easier review. Let me know if you'd like to review in person.