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 #239

Open
TobiasNx opened this issue Jul 4, 2022 · 10 comments
Assignees
Labels
bug Something isn't working conformToCatmandu

Comments

@TobiasNx
Copy link
Collaborator

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: #116 and #195

It seems that array are not copied as arrays at all but each value individually. Therefore #195 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 #116 .

@TobiasNx TobiasNx added the bug Something isn't working label Jul 4, 2022
TobiasNx added a commit that referenced this issue 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
Collaborator Author

TobiasNx commented Jul 5, 2022

This is also connected then: #148 and #92. Am I right?

@TobiasNx
Copy link
Collaborator 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
Collaborator 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
Collaborator 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 #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
Collaborator 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.

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:

https://metafacture.org/playground/?flux=PG_DATA%0A%7Cas-lines%0A%7Cdecode-formeta%0A%7Cfix%0A%7Cencode-xml%28rootTag%3D%22collection%22%29%0A%7Cprint%0A%3B&fix=set_array%28%22test%22%2C%22grey%22%29%0Aif+is_array%28%22test%22%29%0A++++add_field%28%22test.%24append%22%2C%22black%22%29%0Aelsif++is_string%28%22test%22%29%0A++++add_field%28%22test.%24append%22%2C%22white%22%29%0Aend%0A&data=1%7Ba%3A+Faust%2C+b+%7Bn%3A+Goethe%2C+v%3A+JW%7D%2C+c%3A+Weimar%7D%0A2%7Ba%3A+R%C3%A4uber%2C+b+%7Bn%3A+Schiller%2C+v%3A+F%7D%2C+c%3A+Weimar%7D&active-editor=fix

@TobiasNx
Copy link
Collaborator Author

TobiasNx commented Oct 11, 2023

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

if (!Value.isNull(oldValue)) {
oldValue.matchType()
.ifArray(a -> {
record.remove(newName);
a.forEach(v -> record.addNested(newName, v));
})
.orElse(v -> record.set(newName, v));
}

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

@TobiasNx
Copy link
Collaborator 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working conformToCatmandu
Projects
Status: Backlog
Development

No branches or pull requests

3 participants