Skip to content

fix(params): allow zero values for required numeric fields - #4012

Merged
mandre merged 3 commits into
gophercloud:mainfrom
LucasLeao18:fix-registeredlimit-zero-default
Sep 16, 2026
Merged

mandre merged 3 commits into
gophercloud:mainfrom
LucasLeao18:fix-registeredlimit-zero-default

Conversation

@LucasLeao18

@LucasLeao18 LucasLeao18 commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #3866

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

Problem

registeredlimits.CreateOpts.DefaultLimit is declared as int with
required:"true", and BuildRequestBody rejects the zero value of any
required field. As a result the SDK fails client-side with Missing input for argument [DefaultLimit] when creating a registered limit with a
default_limit of 0, even though Keystone accepts 0 as a valid limit
(valid range is -1 to 2147483647).

Solution

BuildRequestBody, BuildQueryString, and BuildHeaders no longer treat a numeric zero as missing input for
fields tagged required:"true". Zero is legitimate input for numeric
fields, so range validation is left to the server. Other zero values
(empty strings, nil pointers, empty structs, ...) are still rejected
client-side as before, so existing validation behavior is unchanged
everywhere else.

This implements option 1 suggested by @winiciusallan in the issue,
scoped to numeric kinds so the existing ErrMissingInput expectations
for string/struct/pointer fields keep working.

Validation

  • Added TestBuildRequestBodyRequiredNumericZero (testing/params_test.go):
    required int zero passes and is serialized; required string empty
    still returns ErrMissingInput.
  • Extended TestBuildQueryString and TestBuildHeaders to verify required
    numeric zero values are serialized while other required zero values remain
    rejected.
  • Added TestCreateRegisteredLimitWithZeroDefaultLimit plus fixtures
    (openstack/identity/v3/registeredlimits/testing/): end-to-end create
    with DefaultLimit: 0, asserting the request body sent on the wire.
  • Verified the new test fails before the fix with exactly the reported
    error (Missing input for argument [DefaultLimit]) and passes after.
  • Full unit suite (go test ./...) passes, except the pre-existing
    TestNormalizePathURL failure which also fails on a clean checkout on
    Windows (path/URL normalization, unrelated to this change).
  • go vet is clean on the touched packages, including the acceptance
    test package (-tags acceptance).
  • Extended the TestRegisteredLimitsCRUD acceptance test with a
    default_limit: 0 create/assert/delete block (not run here, requires a
    live OpenStack cloud).

@github-actions github-actions Bot added edit:gophercloud This PR updates common Gophercloud code edit:identity This PR updates identity code semver:patch No API change backport-v2 This PR will be backported to v2 labels Sep 14, 2026
@coveralls

coveralls commented Sep 15, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 63.61% (+0.02%) from 63.594% — LucasLeao18:fix-registeredlimit-zero-default into gophercloud:main

@mandre mandre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

One question, though. Should we add the same fix for BuildQueryString() and BuildHeaders() that also reject 0 values for numeric when the value is marked as required?

Comment thread internal/acceptance/openstack/identity/v3/registeredlimits_test.go Outdated
@LucasLeao18

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up. I extended the same required numeric-zero handling to BuildQueryString and BuildHeaders, with regression coverage for both, in 9efd285.

@winiciusallan winiciusallan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bom trabalho, @LucasLeao18! I left a few comments inline, please see.

Comment thread params.go Outdated
Comment thread params.go Outdated
Comment thread params.go
@mandre

mandre commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

#4013 should fix the keystone job failure.

@mandre
mandre added this pull request to the merge queue Sep 16, 2026
Merged via the queue into gophercloud:main with commit 79e38f7 Sep 16, 2026
74 of 76 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-v2 This PR will be backported to v2 edit:gophercloud This PR updates common Gophercloud code edit:identity This PR updates identity code semver:patch No API change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RegisteredLimit does not allow a defaultLimit of 0

4 participants