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

The decimal's range should not be parsed if parsing fraction digits fails #153

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

The decimal's range should not be parsed if parsing fraction digits fails #153

Show file tree
Hide file tree
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
3 changes: 2 additions & 1 deletion pkg/yang/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,8 @@ check:
y.IdentityBase = resolvedBase.Identity
}

if t.Range != nil {
// If parsing fraction-digits of type decimal64 fails, the range should not be parsed
if t.Range != nil && (!isDecimal64 || (isDecimal64 && y.FractionDigits != 0)) {
Comment on lines -283 to +284
Copy link
Collaborator

Choose a reason for hiding this comment

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

parseRanges already error checks where FractionDigits is 0 (via decimalValueFromString), so I'm wondering if it's actually better to keep the current behaviour and error out, instead of silently allowing parsing to continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

goyang/pkg/yang/types.go

Lines 247 to 250 in fd7d014

case isDecimal64:
// If we are directly of type decimal64 then we must specify
// fraction-digits in the range from 1-18.
i, err := t.FractionDigits.asRangeInt(1, 18)

In fact, the value 0 indicates that the FractionDigits value has already failed to be parsed (via asRangeInt).

If continue to parse, parserRanges checks the error value 0 as FractionDigits, then reports an unreasonable error msg bad range: invalid number of fraction digits 0: out of range [1..18], and the new testcase also fails because two errors are generated, so I think the value 0 needs to be excluded.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Makes sense.

yr, err := parseRanges(t.Range.Name, isDecimal64, uint8(y.FractionDigits))
switch {
case err != nil:
Expand Down
2 changes: 1 addition & 1 deletion pkg/yang/types_builtin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,7 @@ func ParseDecimal(s string, fracDigRequired uint8) (n Number, err error) {
// numStr must conform to Section 9.3.4.
func decimalValueFromString(numStr string, fracDigRequired uint8) (n Number, err error) {
if fracDigRequired > MaxFractionDigits || fracDigRequired < 1 {
return n, fmt.Errorf("invalid number of fraction digits %d > max of %d, minimum 1", fracDigRequired, MaxFractionDigits)
return n, fmt.Errorf("invalid number of fraction digits %d: out of range [1..%d]", fracDigRequired, MaxFractionDigits)
}

s := numStr
Expand Down
15 changes: 15 additions & 0 deletions pkg/yang/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ func TestTypeResolve(t *testing.T) {
Range: Int64Range,
},
},
{
in: &Type{
Name: "int8",
Range: &Range{Name: "-10 .. 128"},
},
err: "unknown: bad range: -10..128 not within -128..127",
},
{
in: &Type{
Name: "boolean",
Expand All @@ -67,6 +74,14 @@ func TestTypeResolve(t *testing.T) {
},
err: "unknown: value 42 out of range [1..18]",
},
{
in: &Type{
Name: "decimal64",
FractionDigits: &Value{Name: "42"},
Range: &Range{Name: "-10 .. 10"},
},
err: "unknown: value 42 out of range [1..18]",
},
{
in: &Type{
Name: "decimal64",
Expand Down