Refactored the record-data building + post-processing in Objects API registration v2 #4851
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4851 +/- ##
==========================================
- Coverage 96.61% 96.61% -0.01%
==========================================
Files 749 751 +2
Lines 25617 25679 +62
Branches 3394 3395 +1
==========================================
+ Hits 24750 24809 +59
- Misses 604 608 +4
+ Partials 263 262 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
vaszig
approved these changes
Nov 25, 2024
The code was already starting to look like a rather fine Italian pasta
before the addressNL mapping because of component-specific handling of
values that was necessary, but now with the additional configuration
options specific to the component type, readability and maintainability
was in serious danger.
I've opted to look at the broader structure on what information we
need, what information we have and what it is exactly that we need
to do with the information:
* We know all the variable values (keys and value), coming from user
input, derived from user input or entirely independent from user
input.
* When we know the key of a variable and we know the variable source is
a form component, we can look up the definition of the form
component and pass it along as additional context
* We know some user input (file uploads) is processed differently _in
the context of the objects API_ and we have data storage that allows
us to translate
* We need to normalize some values to make them suitable for use in the
Objects API, because a JSON Schema contract is used. Looking at you,
file uploads with multiple=false.
* We may have additional mapping configuration for a specific variable,
typically because the variable points to a particular component type
with special requirements (looking at you, addressNL)
* We must be able to construct the record data for the objects API. The
keys to be used are specified in the mapped variables configuration,
namely the target paths.
* We must process all the mapped variables to construct the entire
object to send to the API.
* We can map all kinds of variables (form, user defined, static,
registration).
So, working backwards, what we needed was for each mapped variable to
make it return instructions which paths of the final object need to
be set to which value. This is solved with AssignmentSpec data classes,
they simply encode the {key: value} information in a well-typed manner.
Next, a single mapping may result in multiple assignments being done
to different parts of the object - this is the newest requirement from
AddressNL type. We can express that as a sequence of AssignmentSpecs.
Finally, we collect all this information, bundle it up and hand it over
to glom which constructs our actual data object, from the mappings.
That leaves us with the requirement of a thing that produces
AssignmentSpec instructions given a mapping configuration and a value.
Because we already know that the value must be transformed depending
on its meaning (=which component produced it), we also pass the
resolved component configuration along for additional context.
For good measure and better code organization, the bulk of the
implementation is moved into handlers/v2.py. More code will be moved
around if this passes the initial reviews.
sergei-maertens
force-pushed
the
refactor/objects-api-v2-postprocessing
branch
from
c88083a to
6c7d33b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #4431 - builds on top of #4850
Changes
Refactored the Objects API v2 registration data building/transformations after the mappings were implemented by @vaszig. I was also able to add more files to the type checker and finally some type definitions / hints could also be cleaned up, hurray!
This had been on my mind for a while when we were fixing the bugs specifically to the file upload types w/r to single/multiple uploads, and now the additional options lead to more 🍝 code in this registration handler, so I figured it was time to put my thoughts into code.