Fix OptionSet variant JSON encoding for empty arrays and scalars#1757
Closed
LivingLikeKrillin wants to merge 1 commit into
Closed
Fix OptionSet variant JSON encoding for empty arrays and scalars#1757LivingLikeKrillin wants to merge 1 commit into
LivingLikeKrillin wants to merge 1 commit into
Conversation
The OPTION_SET branch in encodeVariantValue derived the JSON typeId by
peeking at Array.get(value, 0) on the variant value. This failed in two
real cases:
- Empty arrays threw ArrayIndexOutOfBoundsException
- Single (non-array) OptionSet values threw IllegalArgumentException
("Argument is not an array")
Derive the typeId from the value's class instead, using the existing
OptionSetUI8/UI16/UI32/UI64 subclasses. This is the approach the
original TODO comment proposed.
OptionSetUI64 is supported preemptively even though no DataType in
stack-core currently extends it.
Signed-off-by: Jooyoung Jung <livinglikekrillin@gmail.com>
Contributor
|
Thank you for contributing. I've already fixed this as part of a larger batch of JSON encoding fixes, and your PR just reminded me that I need to push/PR/merge them :) See #1758 So I will close this, but thank you anyway. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
OPTION_SETbranch inOpcUaJsonEncoder.encodeVariantValuederived the JSONtypeIdby callingArray.get(value, 0)on the variant value. This failed in two real cases: empty arrays threwArrayIndexOutOfBoundsException, and single (non-array)OptionSetvalues threwIllegalArgumentException("Argument is not an array"). The latter is reachable via the existing scalar pattern in production code, e.g.new Variant(defaultAccessRestrictions)inNamespaceMetadataTypeNode.Derive the
typeIdfrom the value's class instead, using the existingOptionSetUI8/UI16/UI32/UI64subclasses — the approach the original TODO comment in this branch proposed.OptionSetUI64is supported preemptively even though no DataType in stack-core currently extends it.Test coverage in
OpcUaJsonEncoderTest.encodeVariantOptionSetcovers the width × cardinality matrix for the OptionSet subclasses in current use: scalar UI8, non-empty UI8/UI16/UI32, and empty UI8/UI16/UI32.