Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate RegionFeature originator #497

Closed
wants to merge 1 commit into from

Conversation

floryst
Copy link
Contributor

@floryst floryst commented Sep 6, 2024

@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 floryst requested a review from mvandenburgh September 6, 2024 19:01
@floryst
Copy link
Contributor Author

floryst commented Sep 6, 2024

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
Copy link
Member

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
Copy link
Contributor Author

floryst commented Sep 9, 2024

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants