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

copy_field array of strings with a single value create internally no arrays #621

Open
TobiasNx opened this issue Jul 4, 2022 · 13 comments

Comments

@TobiasNx
Copy link
Contributor

TobiasNx commented Jul 4, 2022

When copying an array of strings that has a single value, fix seems not to copy it as an array but an simple string-element.

This is not spotted since fix outputs simple stringelements that have [] at the end of their name as arrays even if they are not handled as arrays fix-internally. I try to come up with an integration test for this.

Here is an example for it.

This illustrates it even better:

It is disguised by the handling of the output by the json decoder.

This problem could be connected to: metafacture/metafacture-fix#116 and #616

It seems that array are not copied as arrays at all but each value individually. Therefore #616 empty arrays cannot be copied and arrays of strings with single values are not copied as simple string elements. With multiple values the behaviour therefore seems to be identical to the non-destructive behaviour of copy_field/move_field metafacture/metafacture-fix#116 .

@TobiasNx TobiasNx added the Bug label Jul 4, 2022
TobiasNx referenced this issue in metafacture/metafacture-fix Jul 4, 2022
@TobiasNx TobiasNx changed the title copy_field array of strings with a single value copy_field array of strings with a single value are internally no arrays Jul 4, 2022
@TobiasNx TobiasNx changed the title copy_field array of strings with a single value are internally no arrays copy_field array of strings with a single value create internally no arrays Jul 4, 2022
@blackwinter
Copy link
Member

blackwinter commented Jul 5, 2022

copy_field() treats the value as a list (via Value.asList()) because retrieving a path might produce an array where there was none in the data. So it's impossible to preserve the original value's type in the general case.

E.g., with the following internal data:

{ "your": [ { "name": "max" }, { "name": "mo" } ] }

Retrieving your.name would yield an array even though the actual array was your (similar for wildcards, where there might not be any array in the data):

[ "max", "mo" ]

As for your original lookup() issue, there is a workaround of course. I'm just not sure if there's a universal fix to be found here.

@TobiasNx
Copy link
Contributor Author

TobiasNx commented Jul 5, 2022

This is also connected then: #604 and metafacture/metafacture-fix#92. Am I right?

@TobiasNx
Copy link
Contributor Author

TobiasNx commented Jul 5, 2022

Retrieving your.name would yield an array even though the actual array was your (similar for wildcards, where there might not be any array in the data):

[ "max", "mo" ]

I would say this is not the same case. But I do not know if this makes a difference.

While you are interested here in a single element of an array of objects: copy_field("your.*.name")

In my case you are interested in the array as a whole wether it be with a single element, multiple elements or none. copy_field("your[]") or may it be nested copy_field("i.am.nested.your[]")

Also how is catmandu behaving?

@TobiasNx
Copy link
Contributor Author

TobiasNx commented Jul 5, 2022

Also how is catmandu behaving?

https://github.com/TobiasNx/Catmandu-Testing/tree/master/ArrayHandling

It seems that Catmandu is able to work with single and no value array of strings.

@TobiasNx
Copy link
Contributor Author

copy_field() treats the value as a list (via Value.asList()) because retrieving a path might produce an array where there was none in the data. So it's impossible to preserve the original value's type in the general case.
by @blackwinter

One idea I have conceptually is, that you introduce a conditional control if the original value type is an array.
If so, copy_field would create an empty array (like set_array) before copying every single value. This would enable the possibility of copying empty arrays as well as arrays with single values.

@fsteeg
Copy link
Member

fsteeg commented Jul 15, 2022

When copying an array of strings that has a single value, fix seems not to copy it as an array but an simple string-element. [...] fix outputs simple stringelements that have [] at the end of their name as arrays [...]

While I see the conceptual point here, I don't quite see the practical problem. Why not add the []?

And while it might be possible to preserve the original type (based on the path), I don't think it's worth the effort here. We already deviate from Catmandu since we need to specify the array marker in the fix (as in metafacture/metafacture-fix#240), and there is an easy solution, adding the array marker. Internally, the values are always in an array, and the array marker is used precisely to control if we want to output as an array or not.

The empty array is a different topic, and we should fix that, but we have a separate issue for that (#195).

@TobiasNx
Copy link
Contributor Author

TobiasNx commented Jun 15, 2023

copy_field() treats the value as a list (via Value.asList()) because retrieving a path might produce an array where there was none in the data. So it's impossible to preserve the original value's type in the general case.
by @blackwinter

One idea I have conceptually is, that you introduce a conditional control if the original value type is an array. If so, copy_field would create an empty array (like set_array) before copying every single value. This would enable the possibility of copying empty arrays as well as arrays with single values.

Since I used the already established is_array conditional I think this really could be a simple solution for the problem to test if the copied field is_array and if so, create an "new" array first before copying values:

See here.

@TobiasNx
Copy link
Contributor Author

TobiasNx commented Oct 11, 2023

This again popped up. And I am still not sure why this is difficult:

See here

A copied array/list of strings with a single element is not copied as such, but copied as a simple string element.

@TobiasNx
Copy link
Contributor Author

TobiasNx commented Oct 11, 2023

@blackwinter you already introduced a test if the elememt is an array here with metafacture/metafacture-fix@2427f2c:

https://github.com/metafacture/metafacture-fix/blob/68104849904221b6d8fce38aa580945fe39cdd8b/metafix/src/main/java/org/metafacture/metafix/FixMethod.java#L170-L177

would it be possible to adjust the behaviour with single value arrays here too?

@TobiasNx
Copy link
Contributor Author

TobiasNx commented May 6, 2024

Not sure why I am assigned here. I remember that @blackwinter suggested a solution that is not "universal" for every scenario in our meeting. @blackwinter do you remember?

@TobiasNx TobiasNx assigned blackwinter and unassigned TobiasNx May 6, 2024
@TobiasNx
Copy link
Contributor Author

TobiasNx commented Dec 2, 2024

@blackwinter do you remember your suggestion how to approach this problem?

Metafacture Fix should be able to handle arrays with a single index position as arrays:

  • as the incoming JSON/YAML "a": [ "b" ] or "a": [{"b":"c"}] into fix

@blackwinter
Copy link
Member

I don't think there was such a suggestion. I probably said that it cannot work in the general case (because of nested paths and wildcards), but I don't see how we could solve it even for the simple cases.

@blackwinter blackwinter removed their assignment Dec 2, 2024
@blackwinter blackwinter transferred this issue from metafacture/metafacture-fix Jan 27, 2025
@TobiasNx
Copy link
Contributor Author

TobiasNx commented Jan 30, 2025

Thinking about this again there are three scenarios where single value arrays exist and I tested them:

  1. Incoming JSON/Yaml file, that have array markers.
    These arrays are identified as arrays even when the array is empty.
  2. When creating a single value list.
    Created array are identified even with one or none values.
  3. When copy_field a single value list to another field.
    Only this scenario does not work, if I CREATE an array in the fix and copy it. also shows move_field/copy_field of empty arrays does not work #616

This is not only relevant for JSON and YAML handling. It also can be for other testing purposes. Lists/arrays exist within the fix path model too even if the later serialization does not support lists or arrays.

In my case I tried to count an copied array. But since it was not copied the count function did not work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

3 participants