Consolidate path traversal #170

fsteeg · 2022-02-28T14:22:26Z

This is my suggestion to fix #102.

Based on the already merged extraction of FixPath (#133), here are some high-level changes:

Make FixMethod interact with record, which interacts with FixPath (instead of creating FixPaths everywhere)
Consolidate inserting values: replace append/replace methods with insertInto usage
Implement transform methods based on findIn and insertInto to resolve the main issue of Consolidate path traversal. #102
For cases where the findIn path does not work with insertInto (like title-? or some.*.nested.*), use a new Value#path field containing the full path to each value (created by Metafix#flattener)

This fixes some things that didn't work before (and changes the behavior for some things that did not work before and still don't work). See tests diff and issues mentioned in the commit messages.

I think this is an improvement over the current master state, and a good foundation for moving forward. Depending on the current, concrete requirements from our projects, this could be:

Further improve behavior (e.g. handling of repeated fields, wildcards) by revisting the issues that mention #102
Look into performance improvements (improvements in findIn/insertInto will benefit all transformations)
Refactoring this to get it closer to the Catmandu terminology (e.g. new FixPath(field, hash).get())

Causes minor behavior changes (see new @MetafixToDo annotations) Contains comments sketching possible ways to consolidate, basically on two levels: 1) implement transform/insert/remove in terms of find (find does all path traversal, then the result is changed / deleted) 2) move hash / array methods for each operation into hash / array Short-term next step I'd look into: remove / unify InsertMode usage (hence the somewhat odd null / default InsertMode in `insertInto`)

Replace replace*/append* methods with insert* and InsertMode usage Use get / set / add / remove methods on Record for interaction

For cases where the findIn path does not work with insertInto (like `title-?` or `some.*.nested.*`), use a new Value#path field

blackwinter

I'm sorry but there's just too much going on here. c25ede3 breaks our Limetrans workflow in a bunch of different ways (see below) and I'm officially giving up. Also, there seem to be only a few To Dos resolved, but more added. Not really the outcome we were hoping for, is it? (Integration test results as of 6d74bca: 3 fixed, 2 broken.)

So here are just some preliminary remarks and questions, this is not the final assessment.

Behaviour changes observed in Limetrans Alma mapping:

It's unclear where they're coming from but we get null values in records now: Error while executing Fix expression [...]: vacuum(), caused by java.lang.IllegalStateException: Expected Array or Hash or String, got null.
uniq() does not work anymore due to the inclusion of path in Value.equals().
lookup() behaviour seems to have changed unexpectedly (see also Add delete:-Attribute to lookup and change the default filter-modus to an optional configuration #149).
Entity boundaries are violated (probably due to some error/change w.r.t. $append).

metafix/src/main/java/org/metafacture/metafix/FixMethod.java

metafix/src/main/java/org/metafacture/metafix/FixPath.java

metafix/src/main/java/org/metafacture/metafix/Metafix.java

metafix/src/main/java/org/metafacture/metafix/Value.java

metafix/src/test/java/org/metafacture/metafix/MetafixMethodTest.java

Co-authored-by: Jens Wille <[email protected]>

fsteeg · 2022-03-04T15:35:02Z

Thank you for your review and for helping me make sense of all of this @blackwinter!

I've pushed my current state, which addresses many of the issues you brought up.

I'll continue with the remaining ones (capitalize & append/prepend).

blackwinter

Okay, thanks for the update.

build.gradle

metafix/src/main/java/org/metafacture/metafix/FixMethod.java

metafix/src/main/java/org/metafacture/metafix/Value.java

metafix/src/test/java/org/metafacture/metafix/MetafixLookupTest.java

...egration/lookup/fromJson/toJson/lookupSimpleWithInternalMapDeleteNotMatchingLiteral/todo.txt

...e/metafix/integration/record/fromJson/toJson/move_fieldAppendObjectToArrayOfObjects/todo.txt

Co-authored-by: Jens Wille <[email protected]>

fsteeg · 2022-03-07T11:44:39Z

Changed the prepend/append/capitalize/trim tests to the original expected behavior and tweaked the implementation accordingly in be764e4.

Thanks for the additional feedback, addressed it as far as I could for now in 8b27a52.

metafix/src/test/java/org/metafacture/metafix/MetafixLookupTest.java

metafix/src/main/java/org/metafacture/metafix/FixPath.java

metafix/src/test/java/org/metafacture/metafix/MetafixMethodTest.java

metafix/src/main/java/org/metafacture/metafix/FixPath.java

metafix/src/main/java/org/metafacture/metafix/Record.java

Co-authored-by: Jens Wille <[email protected]>

metafix/src/test/java/org/metafacture/metafix/MetafixLookupTest.java

metafix/src/main/java/org/metafacture/metafix/FixPath.java

blackwinter · 2022-04-05T15:36:22Z

Tweaked MetafixScriptTest for order issue in 4961b95.

The behaviour has changed and that should be reflected in the test: 0dfcfb8.

The remaining issue in MetafixScriptTest seems to have the same cause as 7b74937 (not throwing on transform of non-string field).

No, both shouldSkipExpressionOnExecutionException and shouldSkipRecordOnExecutionException are throwing an exception, but should react differently.

metafix/src/test/java/org/metafacture/metafix/MetafixScriptTest.java

To reliably reject string operations on non-string fields

See #170, 0dfcfb8

metafix/src/test/java/org/metafacture/metafix/MetafixMethodTest.java

blackwinter

The moment of truth, huh? 😟 😨 😰

(@MetafixToDo: 23 -> 29, todo.txt: 86 -> 72)

fsteeg · 2022-04-08T14:53:01Z

blackwinter approved these changes

\o/

(@MetafixToDo: 23 -> 29, todo.txt: 86 -> 72)

But we fixed the regressions, these are additional tests. More tests (even failing) are good, no?

Looking at passing tests, I'm very happy: 353 -> 401 (unit), 141 -> 155 (integration).

blackwinter · 2022-04-08T15:01:01Z

But we fixed the regressions, these are additional tests. More tests (even failing) are good, no?

Yes, I was just tying into the previous remarks from way back when this discussion started ;)

…cture-fix#170) Differences in reference files: - Various (inconsequential) order changes.

…c42874) IOW, don't split field names received from metadata stream (`Metafix.addValue()`).

blackwinter · 2022-04-11T10:12:01Z

Found another regression after all :(

Tests and (partial) fix in 1ecbcdf.

…cture-fix#170) Differences in reference files: - Various (inconsequential) order changes.

fsteeg added 5 commits February 28, 2022 14:11

Replace existing values in set_array and set_hash (#102, #111)

1a3278a

Remove appendTo method, hide FixPath behind record (#102, #92, #89)

8c42874

Replace replace*/append* methods with insert* and InsertMode usage Use get / set / add / remove methods on Record for interaction

Implement transformIn using findIn & insertInto (#102, #121, #147)

c25ede3

For cases where the findIn path does not work with insertInto (like `title-?` or `some.*.nested.*`), use a new Value#path field

Move transform methods to Record, avoid FixPath in FixMethod (#102)

692b4f9

fsteeg requested a review from blackwinter February 28, 2022 14:22

fsteeg assigned blackwinter Feb 28, 2022

blackwinter suggested changes Mar 2, 2022

View reviewed changes

blackwinter assigned fsteeg and unassigned blackwinter Mar 2, 2022

fsteeg and others added 6 commits March 4, 2022 08:38

Merge remote-tracking branch 'origin/master' into 102-path

bc59bf4

Exclude .project and metafix.err files in editorconfigChecker

5ec283d

Update passing integration tests (#102, #92, #121, #149, #155)

61e3748

Don't use path in equals, hashCode and toString methods (#102)

34c96f1

Tweak lookup behavior, partial delete support (#102, #149)

e9dd4ab

Integrate feedback from #170 (see #102)

1bcc5ca

Co-authored-by: Jens Wille <[email protected]>

blackwinter reviewed Mar 4, 2022

View reviewed changes

fsteeg and others added 2 commits March 7, 2022 12:25

Throw exception on transform of non-string field (#102, #100, #133)

be764e4

Integrate feedback from #170 (see #102)

8b27a52

Co-authored-by: Jens Wille <[email protected]>

fsteeg requested a review from blackwinter March 7, 2022 12:02

fsteeg assigned blackwinter and unassigned fsteeg Mar 7, 2022

blackwinter suggested changes Mar 7, 2022

View reviewed changes

blackwinter assigned fsteeg and unassigned blackwinter Mar 7, 2022

Some cleanup as suggested in #170 (see #102)

93aaf2e

Co-authored-by: Jens Wille <[email protected]>

blackwinter suggested changes Mar 9, 2022

View reviewed changes

metafix/src/test/java/org/metafacture/metafix/MetafixLookupTest.java Outdated Show resolved Hide resolved

metafix/src/main/java/org/metafacture/metafix/FixPath.java Show resolved Hide resolved

Fix test to actually use internal map (#170, #102)

6f455eb

Update strictness tests for changed behaviour. (#170, 4961b95)

0dfcfb8

fsteeg commented Apr 5, 2022

View reviewed changes

metafix/src/test/java/org/metafacture/metafix/MetafixScriptTest.java Outdated Show resolved Hide resolved

fsteeg commented Apr 5, 2022

View reviewed changes

metafix/src/test/java/org/metafacture/metafix/MetafixScriptTest.java Show resolved Hide resolved

blackwinter added a commit to hbz/Catmandu that referenced this pull request Apr 6, 2022

upcase array testing (metafacture/metafacture-fix#170)

370ca93

fsteeg added 2 commits April 7, 2022 18:50

Replace path heuristic with actual value check in lookup (#170)

2b476e2

To reliably reject string operations on non-string fields

Update strictness integration tests to correspond to unit tests

5e5bf5f

See #170, 0dfcfb8

blackwinter reviewed Apr 8, 2022

View reviewed changes

metafix/src/test/java/org/metafacture/metafix/MetafixMethodTest.java Outdated Show resolved Hide resolved

blackwinter reviewed Apr 8, 2022

View reviewed changes

metafix/src/test/java/org/metafacture/metafix/MetafixMethodTest.java Show resolved Hide resolved

fsteeg added 4 commits April 8, 2022 12:21

Update TODOs on implicit append in unit & integration tests (#170)

540f43f

Improve * wildcard behavior, additional tests (#102, #170)

6779544

Wrap NumberFormatException into IllegalStateException (#170)

746fcf4

Tweak shouldNotTrimIndexedArray, add shouldNotTrimHash test (#170)

9b8fa39

fsteeg requested a review from blackwinter April 8, 2022 13:29

fsteeg assigned blackwinter and unassigned fsteeg Apr 8, 2022

Handle NumberFormatException in apply (#170)

5a3fa9b

blackwinter approved these changes Apr 8, 2022

View reviewed changes

blackwinter assigned fsteeg and unassigned blackwinter Apr 8, 2022

fsteeg merged commit 15e2237 into master Apr 8, 2022

fsteeg deleted the 102-path branch April 8, 2022 14:54

blackwinter added a commit to hbz/limetrans that referenced this pull request Apr 8, 2022

Update for current metafacture-fix master branch. (metafacture/metafa…

e585b0a

…cture-fix#170) Differences in reference files: - Various (inconsequential) order changes.

blackwinter added a commit that referenced this pull request Apr 11, 2022

Only treat field name as FixPath when actually given a path. (#170, 8…

1ecbcdf

…c42874) IOW, don't split field names received from metadata stream (`Metafix.addValue()`).

blackwinter added a commit to hbz/limetrans that referenced this pull request Apr 14, 2022

Update for current metafacture-fix master branch. (metafacture/metafa…

08e5027

…cture-fix#170) Differences in reference files: - Various (inconsequential) order changes.

blackwinter added a commit to hbz/limetrans that referenced this pull request Apr 26, 2022

Update for current metafacture-fix master branch. (metafacture/metafa…

515ee4c

…cture-fix#170) Differences in reference files: - Various (inconsequential) order changes.

blackwinter mentioned this pull request Jul 5, 2022

Add delete:-Attribute to lookup and change the default filter-modus to an optional configuration #149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate path traversal #170

Consolidate path traversal #170

fsteeg commented Feb 28, 2022

blackwinter left a comment •

edited

Loading

fsteeg commented Mar 4, 2022

blackwinter left a comment

fsteeg commented Mar 7, 2022

blackwinter commented Apr 5, 2022

blackwinter left a comment

fsteeg commented Apr 8, 2022

blackwinter commented Apr 8, 2022

blackwinter commented Apr 11, 2022

Consolidate path traversal #170

Consolidate path traversal #170

Conversation

fsteeg commented Feb 28, 2022

blackwinter left a comment • edited Loading

Choose a reason for hiding this comment

fsteeg commented Mar 4, 2022

blackwinter left a comment

Choose a reason for hiding this comment

fsteeg commented Mar 7, 2022

blackwinter commented Apr 5, 2022

blackwinter left a comment

Choose a reason for hiding this comment

fsteeg commented Apr 8, 2022

blackwinter commented Apr 8, 2022

blackwinter commented Apr 11, 2022

blackwinter left a comment •

edited

Loading