fix: Remove required fields that are optional in API responses#559
fix: Remove required fields that are optional in API responses#559erezrokah wants to merge 1 commit intookta:masterfrom
Conversation
|
Hi @pranav-okta, can you point me in the right direction in making these fields non required via the code gen? We're seeing JSON marshalling failures due to these fields being required |
|
Hey @erezrokah, I really appreciate your involvement. To fix this, you need to update the spec and regenerate the SDK. To generate the code, please go through the GitHub action (prepareRelease.yml) and follow the |
erezrokah
force-pushed
the
fix/policy_people_condition
branch
from
a149adf to
9bb5c01
Compare
| with: | ||
| ref: ${{ github.event.inputs.branch }} | ||
|
|
||
| - name: Remove v2 |
There was a problem hiding this comment.
Moved all these steps to the generate makefile target
Done in 9bb5c01 |
erezrokah
force-pushed
the
fix/policy_people_condition
branch
2 times, most recently
from
394c887 to
2637ce7
Compare
|
For context, some of the errors we're seeing are as follows: so potentially there are more fields that needs to be marked as non required (e.g. |
erezrokah
force-pushed
the
fix/policy_people_condition
branch
from
2637ce7 to
350da8c
Compare
The Okta API does not always return grant_types in OIDC app responses, but the OpenAPI spec marks it as required, causing UnmarshalJSON to fail with "no value given for required property grant_types". Applies the same fix pattern introduced in okta#559 (buttonField, allowMultipleAcsEndpoints) to a field that PR did not cover. Verified against a real Okta org: zero unmarshal errors across all apps after this change.
The Okta API does not always return grant_types in OIDC app responses, but the OpenAPI spec marks it as required, causing UnmarshalJSON to fail with "no value given for required property grant_types". Applies the same fix pattern introduced in okta#559 (buttonField, allowMultipleAcsEndpoints) to a field that PR did not cover: - Remove grant_types from the required list in both spec YAML files - Remove from requiredProperties validation in UnmarshalJSON - Add omitempty to the JSON tag - Update constructor and getters accordingly Verified against a real Okta org: zero unmarshal errors across all apps after this change.
|
Hi @erezrokah — thanks for tracking this down and putting together the fix pattern. While testing your PR against our org we found one more field with the same issue: |
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenAPI specs and regenerated Go models to stop treating certain fields as required when the Okta API may omit them in responses, preventing JSON (un)marshaling failures in v6.
Changes:
- Removed
requiredconstraints for specific schemas in the OpenAPI specs and regenerated corresponding Go models to use optional/pointer fields. - Updated generated model docs to reflect optionality and new helper methods (
HasX) / constructor signatures. - Refactored generation workflow to call
make generate, and significantly altered several integration tests (currently into always-skipped stubs).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
okta/api/openapi.yaml |
Removes required lists for affected schemas to match observed API responses. |
.generator/okta-management-APIs-oasv3-noEnums-inheritance.yaml |
Mirrors the same required removals in the generator input spec. |
okta/model_user_identifier_policy_rule_condition.go |
Makes patterns/type optional and removes required-field validation during unmarshal. |
okta/model_swa_application_settings_application.go |
Makes previously required SWA settings fields optional/pointers; adjusts marshal/unmarshal behavior. |
okta/model_saml_application_settings_sign_on.go |
Makes many previously required SAML sign-on fields optional/pointers; adjusts marshal/unmarshal behavior. |
okta/docs/UserIdentifierPolicyRuleCondition.md |
Updates documentation to reflect optional fields and constructor signature change. |
okta/docs/SwaApplicationSettingsApplication.md |
Updates documentation to reflect optional fields and constructor signature change. |
okta/docs/SamlApplicationSettingsSignOn.md |
Updates documentation to reflect optional fields and constructor signature change. |
okta/test/api_user_test.go |
Replaces runnable integration tests with always-skipped stubs and removes environment-guard/cleanup patterns. |
okta/test/api_group_test.go |
Same as above for group tests (skipped stubs, missing request bodies/IDs, missing env guard). |
okta/test/api_application_test.go |
Same as above for application tests (skipped stubs, missing request bodies/IDs, missing env guard). |
Makefile |
Changes generate to a destructive + global-install based flow. |
.github/workflows/prepareReleaseBranch.yml |
Switches release-branch preparation to run make generate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func Test_okta_ApplicationAPIService(t *testing.T) { | ||
| err := okta.ValidateTestEnvironment() | ||
| if err != nil { | ||
| t.Skip("Test environment not configured:", err) | ||
| } | ||
|
|
||
| configuration, err := okta.NewConfiguration() | ||
| configuration, err := openapiclient.NewConfiguration() | ||
| require.Nil(t, err) | ||
| apiClient := okta.NewAPIClient(configuration) | ||
| testDataManager := okta.GetTestDataManager() | ||
| apiClient := openapiclient.NewAPIClient(configuration) | ||
|
|
There was a problem hiding this comment.
NewConfiguration() errors when OKTA_CLIENT_ORGURL isn’t configured (it parses an empty URL). This test now require.Nil(t, err) before any t.Skip, so it will fail instead of being skipped in unconfigured environments. Restore the existing pattern used elsewhere (e.g., ValidateTestEnvironment() + t.Skip(...)) before creating the configuration/client.
| t.Run("Test ApplicationAPIService ActivateApplication", func(t *testing.T) { | ||
|
|
||
| resp, httpRes, err := apiClient.ApplicationAPI.CreateApplication(context.Background()).Application(appRequest).Execute() | ||
| t.Skip("skip test") // remove to run test | ||
|
|
||
| require.Nil(t, err) | ||
| require.NotNil(t, resp) | ||
| assert.Equal(t, http.StatusOK, httpRes.StatusCode) | ||
| var appId string | ||
|
|
||
| assert.NotNil(t, resp.BookmarkApplication) | ||
| assert.NotNil(t, resp.BookmarkApplication.Id) | ||
| assert.Equal(t, "Test Bookmark App", resp.BookmarkApplication.Label) | ||
| httpRes, err := apiClient.ApplicationAPI.ActivateApplication(context.Background(), appId).Execute() | ||
|
|
||
| if resp.BookmarkApplication.Id != nil { | ||
| testDataManager.TrackApplication(*resp.BookmarkApplication.Id) | ||
| } | ||
| }) | ||
| require.Nil(t, err) | ||
| assert.Equal(t, 200, httpRes.StatusCode) | ||
|
|
||
| t.Run("Test ApplicationAPIService ListApplications", func(t *testing.T) { | ||
| // Just test listing applications without creating one first | ||
| // This tests the basic functionality of ListApplications | ||
| _, httpRes, err := apiClient.ApplicationAPI.ListApplications(context.Background()).Execute() | ||
|
|
||
| if err != nil { | ||
| // Handle known JSON unmarshaling issues in the SDK | ||
| if strings.Contains(err.Error(), "failed to unmarshal") && strings.Contains(err.Error(), "data matches more than one schema") { | ||
| t.Logf("Known SDK unmarshaling issue encountered: %v", err) | ||
| // This is acceptable - the API call succeeded but JSON unmarshaling failed | ||
| if httpRes != nil { | ||
| assert.Equal(t, http.StatusOK, httpRes.StatusCode) | ||
| } | ||
| return | ||
| } | ||
| t.Fatalf("Unexpected error: %v", err) | ||
| } | ||
| require.NotNil(t, httpRes) | ||
| assert.Equal(t, http.StatusOK, httpRes.StatusCode) | ||
| // We don't assert on the response contents since we don't know what applications exist | ||
| }) | ||
|
|
||
| t.Run("Test ApplicationAPIService GetApplication", func(t *testing.T) { | ||
| // Create a test application first | ||
| var testFactoryInstance okta.TestFactory | ||
| appRequest := testFactoryInstance.NewValidTestCreateApplicationRequest() | ||
|
|
||
| createdApp, _, createErr := apiClient.ApplicationAPI.CreateApplication(context.Background()).Application(appRequest).Execute() | ||
| require.Nil(t, createErr) | ||
| require.NotNil(t, createdApp) | ||
| require.NotNil(t, createdApp.BookmarkApplication) | ||
| require.NotNil(t, createdApp.BookmarkApplication.Id) | ||
| t.Run("Test ApplicationAPIService CreateApplication", func(t *testing.T) { | ||
|
|
||
| if createdApp.BookmarkApplication.Id != nil { | ||
| testDataManager.TrackApplication(*createdApp.BookmarkApplication.Id) | ||
| } | ||
| t.Skip("skip test") // remove to run test | ||
|
|
||
| resp, httpRes, err := apiClient.ApplicationAPI.GetApplication(context.Background(), *createdApp.BookmarkApplication.Id).Execute() | ||
| resp, httpRes, err := apiClient.ApplicationAPI.CreateApplication(context.Background()).Execute() | ||
|
|
There was a problem hiding this comment.
All subtests are unconditionally skipped and use placeholder IDs / missing request bodies (e.g., CreateApplication(...).Execute() with no .Application(...)). As written, these tests can’t validate the marshaling/unmarshaling behavior this PR is about, and if the skip is removed they will fail with invalid requests. Either restore the prior real API tests (using TestFactory + TestDataManager) or remove these stubs from the test suite.
| func Test_okta_GroupAPIService(t *testing.T) { | ||
| err := okta.ValidateTestEnvironment() | ||
| if err != nil { | ||
| t.Skip("Test environment not configured:", err) | ||
| } | ||
|
|
||
| configuration, err := okta.NewConfiguration() | ||
| configuration, err := openapiclient.NewConfiguration() | ||
| require.Nil(t, err) | ||
| apiClient := okta.NewAPIClient(configuration) | ||
| testDataManager := okta.GetTestDataManager() | ||
|
|
||
| defer func() { | ||
| testDataManager.CleanupAllTestGroups() | ||
| testDataManager.CleanupAllTestUsers() | ||
| }() | ||
| apiClient := openapiclient.NewAPIClient(configuration) | ||
|
|
There was a problem hiding this comment.
NewConfiguration() can fail when env/config isn’t set; this file removed the ValidateTestEnvironment() guard that other integration tests use and now hard-fails with require.Nil(t, err). Please reintroduce ValidateTestEnvironment() + t.Skip(...) and the TestDataManager cleanup/teardown pattern so running go test ./okta/test doesn’t require credentials.
| t.Run("Test GroupAPIService AddGroup", func(t *testing.T) { | ||
| var testFactoryInstance okta.TestFactory | ||
| groupRequest := testFactoryInstance.NewValidTestAddGroupRequest() | ||
|
|
||
| resp, httpRes, err := apiClient.GroupAPI.AddGroup(context.Background()).Group(groupRequest).Execute() | ||
| t.Skip("skip test") // remove to run test | ||
|
|
||
| resp, httpRes, err := apiClient.GroupAPI.AddGroup(context.Background()).Execute() | ||
|
|
||
| require.Nil(t, err) | ||
| require.NotNil(t, resp) | ||
| assert.Equal(t, http.StatusOK, httpRes.StatusCode) | ||
| assert.Equal(t, 200, httpRes.StatusCode) | ||
|
|
||
| assert.NotNil(t, resp.Id) | ||
| assert.NotNil(t, resp.Profile) | ||
|
|
||
| if resp.Profile != nil && resp.Profile.OktaUserGroupProfile != nil { | ||
| assert.Equal(t, *groupRequest.Profile.Name, *resp.Profile.OktaUserGroupProfile.Name) | ||
| assert.Equal(t, *groupRequest.Profile.Description, *resp.Profile.OktaUserGroupProfile.Description) | ||
| } | ||
| }) | ||
|
|
||
| assert.NotNil(t, resp.Created) | ||
| assert.NotNil(t, resp.LastUpdated) | ||
| t.Run("Test GroupAPIService AssignUserToGroup", func(t *testing.T) { | ||
|
|
||
| if resp.Id != nil { | ||
| testDataManager.TrackGroup(*resp.Id) | ||
| } | ||
| }) | ||
| t.Skip("skip test") // remove to run test | ||
|
|
||
| t.Run("Test GroupAPIService GetGroup", func(t *testing.T) { | ||
| createdGroup, err := testDataManager.CreateTestGroup() | ||
| require.Nil(t, err) | ||
| require.NotNil(t, createdGroup) | ||
| require.NotNil(t, createdGroup.Id) | ||
| var groupId string | ||
| var userId string | ||
|
|
||
| resp, httpRes, err := apiClient.GroupAPI.GetGroup(context.Background(), *createdGroup.Id).Execute() | ||
| httpRes, err := apiClient.GroupAPI.AssignUserToGroup(context.Background(), groupId, userId).Execute() | ||
|
|
||
| require.Nil(t, err) | ||
| require.NotNil(t, resp) | ||
| assert.Equal(t, http.StatusOK, httpRes.StatusCode) | ||
|
|
||
| assert.Equal(t, *createdGroup.Id, *resp.Id) | ||
| assert.NotNil(t, resp.Profile) | ||
| assert.Equal(t, 200, httpRes.StatusCode) | ||
|
|
||
| if resp.Profile != nil && resp.Profile.OktaUserGroupProfile != nil && | ||
| createdGroup.Profile != nil && createdGroup.Profile.OktaUserGroupProfile != nil { | ||
| assert.Equal(t, *createdGroup.Profile.OktaUserGroupProfile.Name, *resp.Profile.OktaUserGroupProfile.Name) | ||
| assert.Equal(t, *createdGroup.Profile.OktaUserGroupProfile.Description, *resp.Profile.OktaUserGroupProfile.Description) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Test GroupAPIService ReplaceGroup", func(t *testing.T) { | ||
| createdGroup, err := testDataManager.CreateTestGroup() | ||
| require.Nil(t, err) | ||
| require.NotNil(t, createdGroup) | ||
| require.NotNil(t, createdGroup.Id) | ||
| t.Run("Test GroupAPIService DeleteGroup", func(t *testing.T) { | ||
|
|
||
| var testFactoryInstance okta.TestFactory | ||
| updateProfile := testFactoryInstance.NewTestGroupProfileUpdate() | ||
| updateRequest := okta.NewAddGroupRequest() | ||
| updateRequest.SetProfile(updateProfile) | ||
| t.Skip("skip test") // remove to run test | ||
|
|
||
| resp, httpRes, err := apiClient.GroupAPI.ReplaceGroup(context.Background(), *createdGroup.Id).Group(*updateRequest).Execute() | ||
| var groupId string | ||
|
|
||
| require.Nil(t, err) | ||
| require.NotNil(t, resp) | ||
| assert.Equal(t, http.StatusOK, httpRes.StatusCode) | ||
| httpRes, err := apiClient.GroupAPI.DeleteGroup(context.Background(), groupId).Execute() | ||
|
|
||
| assert.Equal(t, *createdGroup.Id, *resp.Id) | ||
| assert.NotNil(t, resp.Profile) | ||
| require.Nil(t, err) | ||
| assert.Equal(t, 200, httpRes.StatusCode) |
There was a problem hiding this comment.
The previous group tests created real resources and asserted behavior; this version replaces them with always-skipped stubs, placeholder IDs, and missing request bodies (e.g., AddGroup(...).Execute() without .Group(...)). This effectively removes coverage and makes the tests non-runnable. Please keep runnable integration tests (or conditionally skip only when the environment isn’t configured).
| func Test_okta_UserAPIService(t *testing.T) { | ||
|
|
||
| // TestMain provides setup and teardown for all user tests | ||
| func TestMain(m *testing.M) { | ||
| if err := okta.ValidateTestEnvironment(); err != nil { | ||
| log.Printf("Test environment validation failed: %v", err) | ||
| log.Printf("Please set OKTA_CLIENT_ORGURL and OKTA_CLIENT_TOKEN environment variables") | ||
| os.Exit(1) | ||
| } | ||
| configuration, err := openapiclient.NewConfiguration() | ||
| require.Nil(t, err) | ||
| apiClient := openapiclient.NewAPIClient(configuration) | ||
|
|
||
| var err error | ||
| configuration, err := okta.NewConfiguration() | ||
| if err != nil { | ||
| log.Fatalf("Failed to create configuration: %v", err) | ||
| } | ||
| t.Run("Test UserAPIService CreateUser", func(t *testing.T) { |
There was a problem hiding this comment.
This file removed the TestMain/ValidateTestEnvironment setup and now calls NewConfiguration() + require.Nil(t, err) unconditionally. Since NewConfiguration() errors on an empty org URL, this will fail in environments without credentials instead of skipping. Reintroduce ValidateTestEnvironment() (or TestMain setup) so these integration tests safely skip when not configured.
| t.Run("Test UserAPIService CreateUser", func(t *testing.T) { | ||
|
|
||
| apiClient = okta.NewAPIClient(configuration) | ||
| testDataMgr = okta.GetTestDataManager() | ||
| testContext = context.Background() | ||
| t.Skip("skip test") // remove to run test | ||
|
|
||
| exitCode := m.Run() | ||
| resp, httpRes, err := apiClient.UserAPI.CreateUser(context.Background()).Execute() | ||
|
|
||
| log.Println("Cleaning up test resources...") | ||
| testDataMgr.CleanupAllTestUsers() | ||
| require.Nil(t, err) | ||
| require.NotNil(t, resp) | ||
| assert.Equal(t, 200, httpRes.StatusCode) | ||
|
|
||
| os.Exit(exitCode) | ||
| } | ||
| }) | ||
|
|
||
| func Test_okta_UserAPIService(t *testing.T) { | ||
| t.Run("Test UserAPIService DeleteUser", func(t *testing.T) { | ||
|
|
||
| t.Skip("skip test") // remove to run test | ||
|
|
||
| var id string | ||
|
|
||
| httpRes, err := apiClient.UserAPI.DeleteUser(context.Background(), id).Execute() | ||
|
|
||
| require.Nil(t, err) | ||
| assert.Equal(t, 200, httpRes.StatusCode) | ||
|
|
||
| t.Run("Test UserAPIService CreateUser", func(t *testing.T) { | ||
| testFactory := &okta.TestFactory{} | ||
| profile := testFactory.NewValidTestUserProfile() | ||
| credentials := testFactory.NewValidTestUserCredentialsWithPassword() | ||
|
|
||
| createReq := okta.NewCreateUserRequest(profile) | ||
| createReq.SetCredentials(*credentials) | ||
|
|
||
| req := apiClient.UserAPI.CreateUser(testContext).Body(*createReq).Activate(false) | ||
| resp, httpRes, err := req.Execute() | ||
|
|
||
| require.NoError(t, err, "CreateUser should not return an error") | ||
| require.NotNil(t, resp, "Response should not be nil") | ||
| assert.Equal(t, http.StatusOK, httpRes.StatusCode, "HTTP status should be 200") | ||
|
|
||
| assert.NotNil(t, resp.Id, "User ID should be set") | ||
| assert.NotNil(t, resp.Profile, "User profile should be set") | ||
| if resp.Profile != nil { | ||
| assert.Equal(t, profile.GetFirstName(), resp.Profile.GetFirstName(), "First name should match") | ||
| assert.Equal(t, profile.GetLastName(), resp.Profile.GetLastName(), "Last name should match") | ||
| assert.Equal(t, profile.GetEmail(), resp.Profile.GetEmail(), "Email should match") | ||
| } | ||
|
|
||
| if resp.Id != nil { | ||
| testDataMgr.TrackUser(*resp.Id) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Test UserAPIService GetUser", func(t *testing.T) { | ||
| createdUser, err := testDataMgr.CreateTestUser() | ||
| require.NoError(t, err, "Failed to create test user") | ||
| require.NotNil(t, createdUser.Id, "Created user should have an ID") | ||
|
|
||
| resp, httpRes, err := apiClient.UserAPI.GetUser(testContext, *createdUser.Id).Execute() | ||
| t.Skip("skip test") // remove to run test | ||
|
|
||
| var id string | ||
|
|
||
| require.NoError(t, err, "GetUser should not return an error") | ||
| require.NotNil(t, resp, "Response should not be nil") | ||
| assert.Equal(t, http.StatusOK, httpRes.StatusCode, "HTTP status should be 200") | ||
| resp, httpRes, err := apiClient.UserAPI.GetUser(context.Background(), id).Execute() | ||
|
|
||
| require.Nil(t, err) | ||
| require.NotNil(t, resp) | ||
| assert.Equal(t, 200, httpRes.StatusCode) |
There was a problem hiding this comment.
The user API tests were converted from runnable integration tests (creating users via TestFactory and cleaning up via TestDataManager) into always-skipped placeholders with empty IDs and incomplete requests (e.g., CreateUser(...).Execute() without a body). This removes the regression coverage that would catch the marshaling failures mentioned in the PR description. Please restore the prior tests or add focused tests that reproduce the original marshaling issue and assert it’s fixed.
| generate: | ||
| npx @openapitools/openapi-generator-cli generate -c ./.generator/config.yaml -i .generator/okta-management-APIs-oasv3-noEnums-inheritance.yaml --skip-validate-spec | ||
|
|
||
| rm -rf okta openapi tests go.mod go.sum | ||
| npm install @openapitools/openapi-generator-cli -g | ||
| openapi-generator-cli version-manager set 7.15.0 | ||
| openapi-generator-cli generate -c ./.generator/config.yaml -i .generator/okta-management-APIs-oasv3-noEnums-inheritance.yaml --skip-validate-spec | ||
| make fmt | ||
| make import | ||
| cd okta/v6 && mv go.mod go.sum ../../ | ||
| cd okta && mv v6/* ./ && rm -rf v6 |
There was a problem hiding this comment.
make generate now (1) deletes tracked directories/files (rm -rf okta openapi tests go.mod go.sum) and (2) installs openapi-generator-cli globally. This is destructive for local workflows and makes CI less reproducible (global npm state, version-manager side effects). Consider generating into a temp dir and swapping atomically, and prefer a pinned npx @openapitools/openapi-generator-cli@<ver> ... invocation (no global install).
| // NewSamlApplicationSettingsSignOn instantiates a new SamlApplicationSettingsSignOn object | ||
| // This constructor will assign default values to properties that have it defined, | ||
| // and makes sure properties required by API are set, but the set of arguments | ||
| // will change when the set of required properties is changed | ||
| func NewSamlApplicationSettingsSignOn(allowMultipleAcsEndpoints bool, assertionSigned bool, audience string, authnContextClassRef string, destination string, digestAlgorithm string, honorForceAuthn bool, idpIssuer string, recipient string, requestCompressed bool, responseSigned bool, signatureAlgorithm string, ssoAcsUrl string, subjectNameIdFormat string, subjectNameIdTemplate string) *SamlApplicationSettingsSignOn { | ||
| func NewSamlApplicationSettingsSignOn() *SamlApplicationSettingsSignOn { |
There was a problem hiding this comment.
NewSamlApplicationSettingsSignOn(...) changed from a required-args constructor to a no-arg constructor. This is a breaking change for SDK consumers calling the old constructor and/or relying on the previously non-pointer required fields. Please add an explicit migration note (CHANGELOG/MIGRATING) or provide a compatibility helper to reduce upgrade friction.
erezrokah
force-pushed
the
fix/policy_people_condition
branch
from
40647a8 to
d443e8b
Compare
I already signed the CLA a while back for https://github.com/okta/okta-sdk-golang/pulls?q=is%3Apr+author%3Aerezrokah+is%3Aclosed. I can send it again if you don't have it
Summary
Noticed this while working on #558.
Some fields that were optional in v5 are now required in v6 which fails JSON marshaling. Not sure if this is a problem in the OpenAPI spec or in the code gen, but this PR aims to highlight the marshaling failures that happen in v6, but not in v5.
Type of PR
Test Information
Go Version:
go version go1.25.4 darwin/arm64Os Version: MacOS 26.1
OpenAPI Spec Version:
Signoff
make fmton my code