Skip to content

Fix dictionary JSON struct default values#931

Open
jwest115 wants to merge 1 commit intonasa:mainfrom
jwest115:fix/dict-struct-defaults
Open

Fix dictionary JSON struct default values#931
jwest115 wants to merge 1 commit intonasa:mainfrom
jwest115:fix/dict-struct-defaults

Conversation

@jwest115
Copy link
Collaborator

@jwest115 jwest115 commented Feb 26, 2026

  • Takes into account struct member sizes when generating struct value dictionary JSON
  • Clean up some redundant code for array JSON
  • Updated dictionary tests to exercise struct member size case

Closes #925

@jwest115 jwest115 requested a review from bocchino February 26, 2026 22:05
@bocchino
Copy link
Collaborator

This looks good, but now that I see the change, I don't think we want to expand the default values for struct array members. I added a comment to #925. I think we should just write out the default value of the struct, as the value is represented in FPP. If there's some reason that won't work in the GDS, we should try to address it on the GDS side.

val qualifiedName = dictionaryState.a.getQualifiedName(Symbol.Enum(t.node)).toString
s"${qualifiedName}.${v._1}".asJson // FQN of the enum constant
}
case Value.Struct(Value.AnonStruct(members), t) => members.map((key, value) => (key.toString -> valueAsJson(value))).asJson
Copy link
Collaborator

Choose a reason for hiding this comment

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

As noted, the original code looks more correct here. The new code seems to be a special case to implement a difference between FPP semantics and GDS semantics. I'd rather keep those the same, if possible.


res.asJson
}
val arrayRes = for(e <- elements) yield valueAsJson(e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a code improvement (eliminating a special case). Can we keep this change and preserve the existing behavior?

val arrayRes = for(e <- elements) yield valueAsJson(e)
arrayRes.asJson
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed in issue #925, after further discussion, I think we should revert this change. The current behavior is correct according to the way that FPP represents struct values with member arrays. However, @jwest115 can you make sure that the current F Prime JSON spec is consistent with the FPP model and the generated JSON here? This is a tricky case.

Copy link
Collaborator

@bocchino bocchino Mar 2, 2026

Choose a reason for hiding this comment

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

Now I'm not so sure if we should revert. I think the root cause of the issue is that the dictionary JSON is underspecified here. I opened an issue on this: nasa/fprime#4787. There are at least two ways this could work:

  1. When a struct member of type T has a size n, the value stored in the member must be a JSON array with n elements, each of type T. When there is no size, the value must be a single value of type T.

  2. Regardless of whether the struct member has a size, the value stored in the member can be a single value or an array. If there is no size and an array of elements, the array size must be one. If there is a size and a single element, the single element is stored in every position in the array.

Probably we want option (1), but the JSON spec does not specify what behavior is intended. We should tighten up the JSON spec first, and then make the fpp-to-json output conform to it.

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.

Incorrect default value JSON dictionary entry for member arrays of arrays

2 participants