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

Exclude modifications for regional analysis lookup #926

Merged

Conversation

trevorgerhardt
Copy link
Member

This change adds a new helper method to pass a fields object to MongoMap#findByIdIfPermitted and utilizes that to exclude modifications while looking up a regional analysis to generate a scenarioJsonUrl.

This will prevent JacksonDBDecoder errors from occurring if custom modifications are used in a worker version that do not also exist in the server. Sample errors like:

java.lang.RuntimeException: IOException encountered while reading from a byte array input stream
at org.mongojack.internal.stream.JacksonDBDecoder.decode(JacksonDBDecoder.java:67)
at com.conveyal.analysis.persistence.MongoMap.findByIdIfPermitted(MongoMap.java:51)

This change adds a new helper method to pass a `fields` object to `MongoMap#findByIdIfPermitted` and utilizes that to exclude modifications while looking up a regional analysis to generate a `scenarioJsonUrl`.

This will prevent errors from occurring if custom modifications are used in a worker version that do not exist in the server.
Copy link
Member

@abyrd abyrd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed together with @ansoncfit at the Thursday meeting. We talked through the underlying problem.

  • Backend should be able to deserialize unknown types without blowing up (into CustomModificationHolder).
  • Arguably these should be written into the database as type=custom as the original intent was to only ever deserialize custom modifications in a typed fashion on workers.
  • Considered whether it is necessary to save this scenario detail into the database at all if we’re always providing it by fetching this JSON file.
  • Read through Regional analysis persistence cleanup #621 recalling that we have long discussed representing the scenario modifications in persistent storage with the UI types instead of the R5 types, to better decouple them from worker versions.
  • Noted that this PR specifically eliminates the Scenario, while in fact the API endpoint only needs two fields from the entire regional analysis object. So a more basic way to circumvent the problem is to request only the two fields actually needed from the database. However the database wrapper available here focuses solely on fetching into typed objects, probably making that awkward.

Considering all of the above, we decided to go ahead with the approach in this PR, and will create or update an issue to capture the various observations for future reference.

@trevorgerhardt trevorgerhardt merged commit 32eb1dc into dev Jan 26, 2024
3 checks passed
@trevorgerhardt trevorgerhardt deleted the fix-exclude-modifications-from-regional-json-url branch January 26, 2024 10:51
@abyrd
Copy link
Member

abyrd commented Jan 26, 2024

I just tried out replacing DBProjection.exclude("request.scenario.modifications") with DBProjection.include("bundleId", "scenarioId", "accessGroup" to grab only the fields necessary to return the API response. This does work... almost. But the field regionalAnalysis.request.scenarioId is different than regionalAnalysis.scenarioId in the baseline scenario case (UUID versus the string "baseline" respectively). Fetching regionalAnalysis.request.scenarioId means fetching regionalAnalysis.request which runs into other problems with type fields and quickly becomes more trouble than it's worth. The other underlying problem to note here is that we have multiple ways to reference the baseline scenario as a string and mix them together in a single object.

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

Successfully merging this pull request may close these issues.

2 participants