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

Consolidate request and response classes #885

Open
abyrd opened this issue Aug 8, 2023 · 0 comments
Open

Consolidate request and response classes #885

abyrd opened this issue Aug 8, 2023 · 0 comments
Assignees

Comments

@abyrd
Copy link
Member

abyrd commented Aug 8, 2023

For historical reasons, and to support backward compatibility with old worker versions, we have quite a few classes that are very similar representing requests and responses when processing individual accessibility analysis tasks. I noticed when working on #884 that this is a significant obstacle or source of confusion when implementing and testing new kinds of results.

On one hand we have separate classes for the external API and the backend-to-worker communication. On the other hand, we also have separate classes for regional and single point tasks. And some of these classes have layers of superclasses that don't necessarily have any benefit.

There are opportunities to merge five request classes into one or two, and three response classes into one or two, depending on whether separate models are kept for external and internal APIs.

This would be a major change as it would disrupt backward compatibility with workers, and maybe require migration of results or database contents. As such it's something we could think through and document, but would have to delay until we are ready for well-orchestrated major version change.

Request Side

The external API uses a single model for all requests, single point or regional, which is AnalysisRequest. For communication with workers, its populateTask method is used to produce an AnalysisWorkerTask, which has two concrete subtypes: for single point tasks, we create a TravelTimeSurfaceTask in BrokerController, and for regional tasks we create a RegionalTask in RegionalAnalysisController. Also, for historical reasons, AnalysisWorkerTask extends ProfileRequest. It may be possible to merge all five of these classes into a single class, perhaps with some fields factored out into auxiliary classes that are not always included in the request. Such refactoring might also be the occasion to simplify the many small variations that are possible in these requests, for example by always referring to origin and destination point sets by ID and allowing workers to fetch and cache the details of those point sets over an API or from a database, or specifying origin and destination grid sizes separately (the same request fields can currently represent origins or destinations depending on the case).

There may be advantages to merging these into two classes rather than one, where the only distinction is external API vs. internal communication with workers.

Response Side

All accessibility calculations yield an instance of OneOriginResult, a unified internal result type for single point and regional analyses. However, for returning over the API the results are copied into an instance of AnalysisWorker.GridJsonBlock or RegionalWorkResult respectively. These two classes are quite similar in purpose, and there is duplication of logic where they are created from the same source type. This increases maintenance burden and mental load when diagnosing problems, extending or modifying the software. AnalysisWorker.GridJsonBlock ends up being returned via the external API, while RegionalWorkResult only travels between worker and backend where it's collated into regional results, but it seems like the constraints on the representations are similar.

Making a single API model object for use when serializing OneOriginResult and passing it to both the backend and UI would likely break the ability to use old worker versions, as the new normalized representation would differ from the past representation(s).

Aside: Refactoring this might be the occasion to explore other ways of returning multiple data blocks for single point requests: we currently tack a JSON serialization of AnalysisWorker.GridJsonBlock on the end of a binary grid, which makes it tricky to examine and debug the responses. It's not clear that all results should be sent as a single format (e.g. textual representation of numbers) as gridded analysis results are very similar to images, so image formats (or binary formats similar to image formats) are inherently well suited to carry and compress the data, and can be decoded by optimized native code in most browsers. Returning both one or more binary "images" and non-spatial JSON metadata for a single request implies either some kind of multipart response or multiple requests (meaning the server has to be stateful). It might actually be advantageous in other ways to have the server statefully maintain results but that's a real design change.

@abyrd abyrd self-assigned this Aug 8, 2023
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

No branches or pull requests

1 participant