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

fix: resolved inconsistency for empty string csv input #4873

Merged

Conversation

sfiorani
Copy link
Contributor

This PR fixes a bug occurring in the CSV channels upload in the asset section.

If the CSV file contained an empty value for a field that should be casted in string format, it was treated as a "null" value, so the default one from the metatype was set. The correct behaviour should be that an empty csv value should be considered as an empty string, not a null value.

This change can be very impactful on the behavior of all those drivers/assets/channels that read variables in fields like String. Let’s say the case of a string field value which is then casted in the code to an integer with a Integer.decode: at the current state of the art, if this channel is load from csv null field, the validator would set this to a default value from the metatype (for example "10") which would then be casted into an integer; with the modification of this PR, the csv would be read as "" and then would fail the decode because you would try to cast an empty value as integer.

An example of this variation is depicted in the image below: the cannels were loaded from a CSV cotaining all the value field empty (first image). The driver in reading phase, reads the values set in the value channel field and tries to cast it to the value.type type. The metatype set the default value to 0, but passing a null value from the CSV, all values are considered as "", so the different casts fail (second image).

Related Issue: This PR fixes/closes {issue number}

Description of the solution adopted: The validator will now check which is the field type of the CSV field, and if it's a string of type "", it's admitted and not considered as a null value.

Screenshots:
csv_uploaded_channels
csv_uploaded_channels_result

Manual Tests: Tested on Raspberry Pi 3b, Kura version: generic-aarch64

Any side note on the changes made:

@MMaiero
Copy link
Contributor

MMaiero commented Sep 30, 2023

@sfiorani Do we have a way to load an empty string value from csvs?
Can we add a flag in the import modal to let the user select the preferred way to manage empty csv string values?
That would allow us to keep the consistency with the old behaviour

@sfiorani
Copy link
Contributor Author

sfiorani commented Oct 4, 2023

To mantain the compatibility with the previous behaviour, the following steps have been implemented:

  • Added a checkbox in the csv upload popup that allow choosing if the old or new "empty value management" will be follow in the upload
  • Added a short help box for the mentioned new checkbox
  • Reorder of items in the popup
    So now it is possible to choose how to treat empty values in the csv when the field of interest is of String type

@sfiorani sfiorani force-pushed the inconsistent-csv-upload-management branch from 394a9f9 to da8490a Compare October 4, 2023 11:56
@sfiorani sfiorani marked this pull request as ready for review October 4, 2023 11:57
@sfiorani
Copy link
Contributor Author

sfiorani commented Oct 5, 2023

Tested also on Raspberry Pi, Kura version: raspberrypi-arm64

@@ -771,6 +771,8 @@ wiresDownloadChannels=Download Channels
wiresUploadChannels=Upload Channels
wiresNewChannel=New Channel
wiresDeleteChannel=Delete Channel
wiresEmptyStringCheck=Import empty values as empty strings.
wiresEmptyStringCheckHelpText=If checked, empty values contained in the csv will be parsed as empty strings, for all those fields whose type is <String>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If checked, empty values contained in the csv will be parsed as empty strings, for all those fields whose type is

Maybe better like this: "If checked, all typed channel fields that have an empty value in the input csv will be parsed as empty strings."

@@ -771,6 +771,8 @@ wiresDownloadChannels=Download Channels
wiresUploadChannels=Upload Channels
wiresNewChannel=New Channel
wiresDeleteChannel=Delete Channel
wiresEmptyStringCheck=Import empty values as empty strings.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import empty values as empty strings.

Force import empty string policy

@MMaiero
Copy link
Contributor

MMaiero commented Oct 7, 2023

@sfiorani Did you already submit the updated documentation along with this PR?

@sfiorani sfiorani force-pushed the inconsistent-csv-upload-management branch from 1df6839 to 794c580 Compare October 10, 2023 07:31
@MMaiero MMaiero merged commit ecd214e into eclipse-kura:develop Oct 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants