search

ResonantGeoData / RD-WATCH

RD-WATCH Documentation
https://resonantgeodata.github.io/RD-WATCH/
Apache License 2.0
7 stars 5 forks source link

Validate RegionFeature originator #497

Closed floryst closed 2 months ago

floryst commented 2 months ago

@mvandenburgh I discovered that there are two originator fields per region model: one in the RegionFeature and another one in the SiteSummaryFeature. This adds originator validation for the RegionFeature (the previous PR added it for the SiteSummaryFeature).

floryst commented 2 months ago

One perf-related thought: it might be better to move originator validation closer to the root of the schema, since we can do a per-unique originator DB query vs a per-originator field DB query. I noticed that there can be many SiteSummaryFeatures, but I don't know at which point the per-SiteSummaryFeature DB queries become a bottleneck.

mvandenburgh commented 2 months ago

One perf-related thought: it might be better to move originator validation closer to the root of the schema, since we can do a per-unique originator DB query vs a per-originator field DB query. I noticed that there can be many SiteSummaryFeatures, but I don't know at which point the per-SiteSummaryFeature DB queries become a bottleneck.

You're correct. In fact, now that I'm looking at the code, it doesn't seem like we even use the originator field in either the SiteModel or RegionModel. Instead, the performer is set in the ModelRun. The validation check in SiteModel and RegionModel doesn't even make sense to me, as it's just checking if the performer exists, not whether it matches up with the parent ModelRun's performer. We might be able to just remove those validators.

floryst commented 2 months ago

I can remove those validators for now (it simplifies the creation of SiteModels with a performer that doesn't yet exist in #490) and log an issue to validate at the ModelRun level. If you're good with that, I'll close this PR, open a new one to remove the validators, and log the issue.

mvandenburgh commented 2 months ago

I can remove those validators for now (it simplifies the creation of SiteModels with a performer that doesn't yet exist in #490) and log an issue to validate at the ModelRun level. If you're good with that, I'll close this PR, open a new one to remove the validators, and log the issue.

Yeah that sounds good to me