Fix accidental mutation side effect breaking the Objects API v1 registration #5043
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5043 +/- ##
=======================================
Coverage 96.71% 96.71%
=======================================
Files 770 770
Lines 26488 26489 +1
Branches 3453 3453
=======================================
+ Hits 25617 25618 +1
Misses 607 607
Partials 264 264 ☔ View full report in Codecov by Sentry. |
sergei-maertens
changed the title
vaszig
approved these changes
Jan 28, 2025
| @@ -36,12 +37,16 @@ const VariablesEditor = ({variables, onAdd, onDelete, onChange, onFieldChange}) | |||
| <Tabs> | |||
| <TabList> | |||
| <Tab hasErrors={componentVariables.some(variable => variableHasErrors(variable))}> | |||
| {intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.component)} | |||
| {intl.formatMessage(VARIABLE_SOURCES_GROUP_LABELS.component)} ( | |||
| {componentVariables.length}) | |||
…cords Components nested in repeating groups are not real variables. The frontend code showed these in the variables table AND this also caused them to be PUT to the backend, resulting in crashes when processing file uploads because there are no saved submission variables available for them. Unfortunately, this doesn't appear to fix the problem with too much keys being included in the submission data during registration, so it must have a different root cause.
This is available on the model too, so we can build on top of that.
Damn you, side effects.
…ep configuration The total configuration wrapper merges the configuration wrapper of each step into a single object for optimized access to values/ components. It takes the first step and merges the remaining steps into it. However, this had the unintended side-effect of mutating the config of the first step, manifesting in the objects API v1 registration with the json_summary tag which contained extra, unexpected keys in the submission data of the first step. Fixed by making a deep copy first to end up with a different instance that can be safely mutated.
sergei-maertens
force-pushed
the
issue/5035-duplicated-json-summary-nodes
branch
from
2bbfe66 to
9e90a4c
Compare
sergei-maertens
added a commit
that referenced
this pull request
Jan 28, 2025
Fix 1: Components nested in repeating groups are not real variables. The frontend code showed these in the variables table AND this also caused them to be PUT to the backend, resulting in crashes when processing file uploads because there are no saved submission variables available for them. Unfortunately, this doesn't appear to fix the problem with too much keys being included in the submission data during registration, so it must have a different root cause. Fix 2: Avoid the total configuration wrapper mutating the first step configuration The total configuration wrapper merges the configuration wrapper of each step into a single object for optimized access to values/ components. It takes the first step and merges the remaining steps into it. However, this had the unintended side-effect of mutating the config of the first step, manifesting in the objects API v1 registration with the json_summary tag which contained extra, unexpected keys in the submission data of the first step. Fixed by making a deep copy first to end up with a different instance that can be safely mutated. Backport-of: #5043
sergei-maertens
added
the
needs-backport
sergei-maertens
added a commit
that referenced
this pull request
Jan 28, 2025
Fix 1: Components nested in repeating groups are not real variables. The frontend code showed these in the variables table AND this also caused them to be PUT to the backend, resulting in crashes when processing file uploads because there are no saved submission variables available for them. Unfortunately, this doesn't appear to fix the problem with too much keys being included in the submission data during registration, so it must have a different root cause. Fix 2: Avoid the total configuration wrapper mutating the first step configuration The total configuration wrapper merges the configuration wrapper of each step into a single object for optimized access to values/ components. It takes the first step and merges the remaining steps into it. However, this had the unintended side-effect of mutating the config of the first step, manifesting in the objects API v1 registration with the json_summary tag which contained extra, unexpected keys in the submission data of the first step. Fixed by making a deep copy first to end up with a different instance that can be safely mutated. Backport-of: #5043
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.
Closes #5035
Changes
FormVariablerecords being created for components inside repeating groupsThe regression test + bugfix can be cherrypicked to the stable/3.0.x branch for backport purposes, the rest is for the main branch.
Checklist
Check off the items that are completed or not relevant.
Impact on features
Release management
I have updated the translations assets (you do NOT need to provide translations)
./bin/makemessages_js.sh./bin/compilemessages_js.shDockerfile/scripts
./binfolderCommit hygiene