[FW][FIX] DataFilter: Fix overlapping filters #3630
Closed
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
The fix proposed in PR #2631 was incomplete. While it prevented the *new* duplication of filter tables on overlapping zones (due to a mismatch inthe plugin internal mapping) it didn't fix the filters already duplicated and overlapping. This revision adds a new migration to clear the duplicated filters based on the knowledge of how the bug could occur. Before #2631, when duplicating a sheet containing a filter, the filter plugin would: - Deep copy the entry of `this.tables[cmd.sheetIdFrom]` - an Object mapping the id of a `FilterTable` with the corresponding `FilterTable` object - create a new entry for `cmd.sheetIdTo` in `this.tables` with the deep copy However, deep copy would create a new `FilterTable` object with a new generated id (type uuid) which would mismatch with the internal mapping of the copied object. e.g. ```typescript // in sheet1 table1 = {id1: filter1 instanceof FilterTable} id1 === filter1.id // in sheet2 table2 = deepCopy(table1) table2 === {id1: filter2 instanceof FilterTable} id1 !== filter2.id ``` When trying to then alter the copied sheet structure and such modification altered the filters, the implementation did not account for such situation and would end up duplicating the filter table in `sheet2`, thus creating a sheet with filters with overlapping zones. E.g. ``` typescript tables["sheet2"] = {id1: filter2} filter2.zone === "A1:A10" deleteRow(5) tables["sheet2"] = {id1: filter2, id3:filter3} id1 !== filter2.id id3 === filter3.id // overlap in filte zones filter2.zone === "A1:A10" filter3.zone === "A1:A9" ``` Since Javascript guarantee `Object` property insertion order upon iteration (for equivalent objects [1]), we can safely say that in such situation the latest key of the object `tables["sheet2"]` is the truth and we can delete the other keyslinked to filters with overlapping zones. One of the side-effects of these overlapping tables was a crash in the owl component `FilterIconsOverlay` due to some duplicated keys. As such, a regression test was added to cover this case. [1] https://262.ecma-international.org/10.0/ Task: 3728009 X-original-commit: be0fda3
|
This PR targets saas-16.1 and is part of the forward-port chain. Further PRs will be created up to master. More info at https://github.com/odoo/odoo/wiki/Mergebot#forward-port |
robodoo
pushed a commit
that referenced
this pull request
Feb 7, 2024
The fix proposed in PR #2631 was incomplete. While it prevented the *new* duplication of filter tables on overlapping zones (due to a mismatch inthe plugin internal mapping) it didn't fix the filters already duplicated and overlapping. This revision adds a new migration to clear the duplicated filters based on the knowledge of how the bug could occur. Before #2631, when duplicating a sheet containing a filter, the filter plugin would: - Deep copy the entry of `this.tables[cmd.sheetIdFrom]` - an Object mapping the id of a `FilterTable` with the corresponding `FilterTable` object - create a new entry for `cmd.sheetIdTo` in `this.tables` with the deep copy However, deep copy would create a new `FilterTable` object with a new generated id (type uuid) which would mismatch with the internal mapping of the copied object. e.g. ```typescript // in sheet1 table1 = {id1: filter1 instanceof FilterTable} id1 === filter1.id // in sheet2 table2 = deepCopy(table1) table2 === {id1: filter2 instanceof FilterTable} id1 !== filter2.id ``` When trying to then alter the copied sheet structure and such modification altered the filters, the implementation did not account for such situation and would end up duplicating the filter table in `sheet2`, thus creating a sheet with filters with overlapping zones. E.g. ``` typescript tables["sheet2"] = {id1: filter2} filter2.zone === "A1:A10" deleteRow(5) tables["sheet2"] = {id1: filter2, id3:filter3} id1 !== filter2.id id3 === filter3.id // overlap in filte zones filter2.zone === "A1:A10" filter3.zone === "A1:A9" ``` Since Javascript guarantee `Object` property insertion order upon iteration (for equivalent objects [1]), we can safely say that in such situation the latest key of the object `tables["sheet2"]` is the truth and we can delete the other keyslinked to filters with overlapping zones. One of the side-effects of these overlapping tables was a crash in the owl component `FilterIconsOverlay` due to some duplicated keys. As such, a regression test was added to cover this case. [1] https://262.ecma-international.org/10.0/ closes #3630 Task: 3728009 X-original-commit: be0fda3 Signed-off-by: Lucas Lefèvre (lul) <[email protected]> Signed-off-by: Rémi Rahir (rar) <[email protected]>
Topdev97
added a commit
to Topdev97/o-spreadsheet
that referenced
this pull request
Nov 18, 2024
The fix proposed in PR #2631 was incomplete. While it prevented the
*new* duplication of filter tables on overlapping zones (due to a mismatch
inthe plugin internal mapping) it didn't fix the filters already
duplicated and overlapping.
This revision adds a new migration to clear the duplicated filters based
on the knowledge of how the bug could occur.
Before #2631, when duplicating a sheet containing a filter, the filter
plugin would:
- Deep copy the entry of `this.tables[cmd.sheetIdFrom]` - an Object
mapping the id of a `FilterTable` with the corresponding
`FilterTable` object
- create a new entry for `cmd.sheetIdTo` in `this.tables` with the deep
copy
However, deep copy would create a new `FilterTable` object with a new
generated id (type uuid) which would mismatch with the internal mapping
of the copied object.
e.g.
```typescript
// in sheet1
table1 = {id1: filter1 instanceof FilterTable}
id1 === filter1.id
// in sheet2
table2 = deepCopy(table1)
table2 === {id1: filter2 instanceof FilterTable}
id1 !== filter2.id
```
When trying to then alter the copied sheet structure and such
modification altered the filters, the implementation did not account for
such situation and would end up duplicating the filter table in `sheet2`,
thus creating a sheet with filters with overlapping zones.
E.g.
``` typescript
tables["sheet2"] = {id1: filter2}
filter2.zone === "A1:A10"
deleteRow(5)
tables["sheet2"] = {id1: filter2, id3:filter3}
id1 !== filter2.id
id3 === filter3.id
// overlap in filte zones
filter2.zone === "A1:A10"
filter3.zone === "A1:A9"
```
Since Javascript guarantee `Object` property insertion order upon
iteration (for equivalent objects [1]), we can safely say that in such
situation the latest key of the object `tables["sheet2"]` is the truth
and we can delete the other keyslinked to filters with overlapping
zones.
One of the side-effects of these overlapping tables was a crash in the
owl component `FilterIconsOverlay` due to some duplicated keys. As such,
a regression test was added to cover this case.
[1] https://262.ecma-international.org/10.0/
closes odoo/o-spreadsheet#3630
Task: 3728009
X-original-commit: be0fda3780f326ebc5a275396f0b48384a1e3c0e
Signed-off-by: Lucas Lefèvre (lul) <[email protected]>
Signed-off-by: Rémi Rahir (rar) <[email protected]>
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.
The fix proposed in PR #2631 was incomplete. While it prevented the new duplication of filter tables on overlapping zones (due to a mismatch inthe plugin internal mapping) it didn't fix the filters already duplicated and overlapping.
This revision adds a new migration to clear the duplicated filters based on the knowledge of how the bug could occur.
Before #2631, when duplicating a sheet containing a filter, the filter plugin would:
this.tables[cmd.sheetIdFrom]- an Object mapping the id of aFilterTablewith the correspondingFilterTableobjectcmd.sheetIdTointhis.tableswith the deep copyHowever, deep copy would create a new
FilterTableobject with a new generated id (type uuid) which would mismatch with the internal mapping of the copied object.e.g.
When trying to then alter the copied sheet structure and such modification altered the filters, the implementation did not account for such situation and would end up duplicating the filter table in
sheet2, thus creating a sheet with filters with overlapping zones.E.g.
Since Javascript guarantee
Objectproperty insertion order upon iteration (for equivalent objects [1]), we can safely say that in such situation the latest key of the objecttables["sheet2"]is the truth and we can delete the other keyslinked to filters with overlapping zones.One of the side-effects of these overlapping tables was a crash in the owl component
FilterIconsOverlaydue to some duplicated keys. As such, a regression test was added to cover this case.[1] https://262.ecma-international.org/10.0/
Task: 3728009
Description:
description of this task, what is implemented and why it is implemented that way.
Task: : TASK_ID
review checklist
Forward-Port-Of: #3629