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.

Sign up for GitHub

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

Jump to bottom

Correctly send empty array to astarte #247

Open
wants to merge 1 commit into
base: release-23.5
Choose a base branch
from
Open

Correctly send empty array to astarte #247

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 32 additions & 21 deletions cmd/appengine/device.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1603,6 +1603,7 @@ func parseSendDataPayload(payload string, mappingType interfaces.AstarteMappingT
// Default to string, as it will be ok for most cases
var ret interface{} = payload
var err error

switch mappingType {
case interfaces.Double:
if ret, err = strconv.ParseFloat(payload, 64); err != nil {
Expand Down Expand Up @@ -1634,32 +1635,42 @@ func parseSendDataPayload(payload string, mappingType interfaces.AstarteMappingT
case interfaces.BinaryBlobArray, interfaces.BooleanArray, interfaces.DateTimeArray, interfaces.DoubleArray,
interfaces.IntegerArray, interfaces.LongIntegerArray, interfaces.StringArray:

//wait it's all string?
payload = strings.Replace(payload, "[", "", -1)
payload = strings.Replace(payload, "]", "", -1)
payload_parsed := strings.Split(payload, ",")
//always has been
//check if payload is an empty array, bypass conversions and just return an empty array
// if it is not, proceed as usual
if payload == "[]" {
ret = make([]interface{}, 0)

jsonOut := make([]interface{}, len(payload_parsed))
for i, v := range payload_parsed {
jsonOut[i] = v
}
} else {

retArray := []interface{}{}
// Do a smarter conversion here.
for _, v := range jsonOut {
switch val := v.(type) {
case string:
p, err := parseSendDataPayload(strings.TrimSpace(val), interfaces.AstarteMappingType(strings.TrimSuffix(string(mappingType), "array")))
if err != nil {
return nil, err
//wait it's all string?
payload = strings.Replace(payload, "[", "", -1)
payload = strings.Replace(payload, "]", "", -1)
payload_parsed := strings.Split(payload, ",")
//always has been
Comment on lines +1640 to +1649
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest using a different approach for parsing the array data. We know that:

  • The input payload has the form [<comma separated data>]
  • A [<comma separated data>] value is a valid JSON object
  • Go's json.Unmarshal is able to handle these objects (see https://go.dev/play/p/ixaicx2S7cj).

Therefore, my suggestion is to let Go handle characters such as [ and just do the conversion of payloads, something like:

var jsonPayload []interface{}
if err := json.Unmarshal([]byte(payload), &jsonPayload); err != nil {
	fmt.Println(err) //Maybe also warn the user of the bad input payload
	System.exit(1)
}
retArray := []interface{}{}

// Do a smarter conversion here.
for _, v := range jsonPayload {
// etc etc, the rest is the same

This would mean that the user will have to write a valid JSON payload, e.g. strings inside arrays should be put in double quotes, like astartectl appengine devices publish-datastream <device_id> <interface> <path> '["a", "b", "c"]'

Copy link
Contributor Author

@eddbbt eddbbt May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sounds good, but we need to check that requiring an array of explicit strings ('["a", "b", "c"]') will not break compatibility with already existent implementations (if any).
Current string interpolation was added in #235


jsonOut := make([]interface{}, len(payload_parsed))
for i, v := range payload_parsed {
jsonOut[i] = v
}

retArray := []interface{}{}

// Do a smarter conversion here.
for _, v := range jsonOut {
switch val := v.(type) {
case string:
p, err := parseSendDataPayload(strings.TrimSpace(val), interfaces.AstarteMappingType(strings.TrimSuffix(string(mappingType), "array")))
if err != nil {
return nil, err
}
retArray = append(retArray, p)
default:
retArray = append(retArray, val)
Comment on lines +1667 to +1668
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we ever reach this point? I'm not sure, and in case we hit this line we'd append a v.(type) in retArray (that looks strange).
Is it reasonable assuming that within the parseSendDataPayload, when dealing with arrays, we always deal with strings? A double check is needed here. In case that assumption is true, we could remove the switch construct.
@Annopaolo opinions here?

Copy link
Contributor Author

@eddbbt eddbbt Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

when dealing with arrays, we always deal with strings?

looks like that's the case, command line always passes a string and that's why this problem arise

Copy link
Collaborator

Choose a reason for hiding this comment

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

A type assertion could be used then, as @matt-mazzucato suggested

}
retArray = append(retArray, p)
default:
retArray = append(retArray, val)
}
ret = retArray
}
ret = retArray

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not add extra lines if not needed.

}

return ret, nil
Expand Down
Loading