From 0a10bc3f6c502524525a6810e486f42884af8ac3 Mon Sep 17 00:00:00 2001 From: Tanvir Ahmed Date: Sat, 10 Oct 2026 00:15:23 +0000 Subject: [PATCH] fix: Organization logo is now cleared when update sends explicit null The update-organization endpoint could not tell an explicit logo: null apart from an omitted logo field, so clearing a logo left the old value in place. UpdateOrganizationRequest now records whether the logo field was present in the incoming JSON and the service clears the logo to nil for an explicit null or empty string. --- core/types/optional.go | 63 +++++++++++++++++ core/types/optional_test.go | 69 +++++++++++++++++++ .../handlers/organization_handlers_test.go | 27 +++++++- .../services/organization_service.go | 8 ++- .../services/organization_service_test.go | 27 ++++++++ plugins/organizations/types/api.go | 15 ++-- 6 files changed, 198 insertions(+), 11 deletions(-) create mode 100644 core/types/optional.go create mode 100644 core/types/optional_test.go diff --git a/core/types/optional.go b/core/types/optional.go new file mode 100644 index 00000000..09588d6c --- /dev/null +++ b/core/types/optional.go @@ -0,0 +1,63 @@ +package types + +import ( + "encoding/json" + "reflect" + + "github.com/swaggest/jsonschema-go" +) + +// Optional represents a JSON object field that may be absent, present with a +// null value, or present with a concrete value. The zero value is "absent". +// +// It is used on PATCH-style request structs to tell an omitted field apart from +// a field explicitly set to null, which a plain pointer cannot do. +// +// Use the `omitzero` JSON tag so an absent field is left out when encoding. +type Optional[T any] struct { + Value T + Present bool +} + +// UnmarshalJSON records that the field was present and decodes its value. +func (o *Optional[T]) UnmarshalJSON(data []byte) error { + o.Present = true + + var zero T + o.Value = zero + + return json.Unmarshal(data, &o.Value) +} + +// MarshalJSON encodes the underlying value. An absent optional is omitted from +// the object when the field uses the `omitzero` tag. +func (o Optional[T]) MarshalJSON() ([]byte, error) { + return json.Marshal(o.Value) +} + +// IsZero reports whether the field was absent so that encoding/json can omit it +// when the `omitzero` option is used. +func (o Optional[T]) IsZero() bool { + return !o.Present +} + +// PrepareJSONSchema makes the generated schema describe the inner type T +// instead of the wrapper struct. This keeps Optional invisible to API clients. +// +// It works for scalar, pointer, and map inner types. Named struct inner types +// can produce references that the outer reflector does not collect. +func (o Optional[T]) PrepareJSONSchema(schema *jsonschema.Schema) error { + innerType := reflect.TypeFor[T]() + + var reflector jsonschema.Reflector + reflector.InlineDefinition(reflect.Zero(innerType).Interface()) + + inner, err := reflector.Reflect(reflect.Zero(innerType).Interface()) + if err != nil { + return err + } + + *schema = inner + + return nil +} diff --git a/core/types/optional_test.go b/core/types/optional_test.go new file mode 100644 index 00000000..481abe90 --- /dev/null +++ b/core/types/optional_test.go @@ -0,0 +1,69 @@ +package types_test + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + coretypes "github.com/Authula/authula/core/types" +) + +func TestOptional_Unmarshal(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + body string + wantPresent bool + wantValue *string + }{ + {name: "absent", body: `{}`, wantPresent: false, wantValue: nil}, + {name: "null", body: `{"logo":null}`, wantPresent: true, wantValue: nil}, + {name: "empty string", body: `{"logo":""}`, wantPresent: true, wantValue: new("")}, + {name: "value", body: `{"logo":"http://some/url/logo.svg"}`, wantPresent: true, wantValue: new("http://some/url/logo.svg")}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + var payload struct { + Logo coretypes.Optional[*string] `json:"logo,omitzero"` + } + require.NoError(t, json.Unmarshal([]byte(tt.body), &payload)) + + assert.Equal(t, tt.wantPresent, payload.Logo.Present) + assert.Equal(t, tt.wantValue, payload.Logo.Value) + }) + } +} + +func TestOptional_Marshal(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + value coretypes.Optional[*string] + want string + }{ + {name: "absent is omitted", value: coretypes.Optional[*string]{}, want: `{}`}, + {name: "null is kept", value: coretypes.Optional[*string]{Present: true, Value: nil}, want: `{"logo":null}`}, + {name: "value is kept", value: coretypes.Optional[*string]{Present: true, Value: new("http://some/url/logo.svg")}, want: `{"logo":"http://some/url/logo.svg"}`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + payload := struct { + Logo coretypes.Optional[*string] `json:"logo,omitzero"` + }{Logo: tt.value} + + data, err := json.Marshal(payload) + require.NoError(t, err) + assert.JSONEq(t, tt.want, string(data)) + }) + } +} diff --git a/plugins/organizations/handlers/organization_handlers_test.go b/plugins/organizations/handlers/organization_handlers_test.go index 366388c3..3769b1e6 100644 --- a/plugins/organizations/handlers/organization_handlers_test.go +++ b/plugins/organizations/handlers/organization_handlers_test.go @@ -12,6 +12,7 @@ import ( coreerrors "github.com/Authula/authula/core/errors" "github.com/Authula/authula/core/pagination" + coretypes "github.com/Authula/authula/core/types" internaltests "github.com/Authula/authula/internal/tests" "github.com/Authula/authula/models" orgconstants "github.com/Authula/authula/plugins/organizations/constants" @@ -410,7 +411,7 @@ func TestUpdateOrganizationHandler(t *testing.T) { organizationID: "org-1", body: internaltests.MarshalToJSON(t, orgtypes.UpdateOrganizationRequest{ Name: new("Acme Platform"), - Logo: new("http://some/url/logo.svg"), + Logo: coretypes.Optional[*string]{Present: true, Value: new("http://some/url/logo.svg")}, Metadata: map[string]any{"tier": "pro"}, }), prepare: func(f *organizationHandlerFixture) { @@ -443,7 +444,7 @@ func TestUpdateOrganizationHandler(t *testing.T) { organizationID: "org-1", body: internaltests.MarshalToJSON(t, orgtypes.UpdateOrganizationRequest{ Name: new("Acme Platform"), - Logo: new("http://some/url/logo.svg"), + Logo: coretypes.Optional[*string]{Present: true, Value: new("http://some/url/logo.svg")}, Metadata: map[string]any{"tier": "pro"}, }), prepare: func(f *organizationHandlerFixture) { @@ -470,6 +471,28 @@ func TestUpdateOrganizationHandler(t *testing.T) { assert.JSONEq(t, "{\"tier\":\"pro\"}", string(internaltests.MarshalToJSON(t, org.Metadata))) }, }, + { + name: "explicit null logo is treated as logo provided but resets the logo to nil", + userID: new("user-1"), + organizationID: "org-1", + body: []byte(`{"logo":null}`), + prepare: func(f *organizationHandlerFixture) { + f.service.On("UpdateOrganization", mock.Anything, "user-1", "org-1", mock.MatchedBy(func(request orgtypes.UpdateOrganizationRequest) bool { + return request.Logo.Present && request.Logo.Value == nil + })).Return(&orgtypes.Organization{ + ID: "org-1", + OwnerID: "user-1", + Name: "Acme", + Slug: "acme", + Logo: nil, + }, nil).Once() + }, + expectedStatus: http.StatusOK, + checkResponse: func(t *testing.T, reqCtx *models.RequestContext) { + org := internaltests.DecodeResponseJSON[orgtypes.Organization](t, reqCtx) + assert.Nil(t, org.Logo) + }, + }, } for _, tt := range tests { diff --git a/plugins/organizations/services/organization_service.go b/plugins/organizations/services/organization_service.go index e398b737..332bd63a 100644 --- a/plugins/organizations/services/organization_service.go +++ b/plugins/organizations/services/organization_service.go @@ -263,8 +263,12 @@ func (s *organizationService) UpdateOrganization(ctx context.Context, actor *mod organization.Name = *name } organization.Slug = slug - if request.Logo != nil { - organization.Logo = request.Logo + if request.Logo.Present { + if request.Logo.Value == nil || *request.Logo.Value == "" { + organization.Logo = nil + } else { + organization.Logo = request.Logo.Value + } } if request.Metadata != nil { organization.Metadata = request.Metadata diff --git a/plugins/organizations/services/organization_service_test.go b/plugins/organizations/services/organization_service_test.go index b05f1cde..e0200221 100644 --- a/plugins/organizations/services/organization_service_test.go +++ b/plugins/organizations/services/organization_service_test.go @@ -10,6 +10,7 @@ import ( coreerrors "github.com/Authula/authula/core/errors" "github.com/Authula/authula/core/pagination" + coretypes "github.com/Authula/authula/core/types" internaltests "github.com/Authula/authula/internal/tests" "github.com/Authula/authula/models" "github.com/Authula/authula/plugins/organizations/constants" @@ -530,6 +531,32 @@ func TestOrganizationService_UpdateOrganization(t *testing.T) { })).Return(&types.Organization{ID: "org-1", OwnerID: "user-1", Name: "Acme Platform", Slug: "acme"}, nil).Once() }, }, + { + name: "explicit null logo clears it", + actorUserID: "user-1", + organizationID: "org-1", + request: types.UpdateOrganizationRequest{Logo: coretypes.Optional[*string]{Present: true, Value: nil}}, + setup: func(repo *orgtests.MockOrganizationRepository, memberRepo *orgtests.MockOrganizationMemberRepository, hooks *orgtests.MockOrganizationHooks, serviceUtils *ServiceUtils) { + repo.On("GetByID", mock.Anything, "org-1").Return(&types.Organization{ID: "org-1", OwnerID: "user-1", Name: "Acme", Slug: "acme", Logo: new("http://old/logo.svg")}, nil).Once() + memberRepo.On("GetByOrganizationIDAndUserID", mock.Anything, "org-1", "user-1").Return(&types.OrganizationMember{ID: "mem-1", OrganizationID: "org-1", UserID: "user-1", Role: "member"}, nil).Once() + repo.On("Update", mock.Anything, mock.MatchedBy(func(org *types.Organization) bool { + return org != nil && org.ID == "org-1" && org.Logo == nil + })).Return(&types.Organization{ID: "org-1", OwnerID: "user-1", Name: "Acme", Slug: "acme", Logo: nil}, nil).Once() + }, + }, + { + name: "empty string logo clears it", + actorUserID: "user-1", + organizationID: "org-1", + request: types.UpdateOrganizationRequest{Logo: coretypes.Optional[*string]{Present: true, Value: new("")}}, + setup: func(repo *orgtests.MockOrganizationRepository, memberRepo *orgtests.MockOrganizationMemberRepository, hooks *orgtests.MockOrganizationHooks, serviceUtils *ServiceUtils) { + repo.On("GetByID", mock.Anything, "org-1").Return(&types.Organization{ID: "org-1", OwnerID: "user-1", Name: "Acme", Slug: "acme", Logo: new("http://old/logo.svg")}, nil).Once() + memberRepo.On("GetByOrganizationIDAndUserID", mock.Anything, "org-1", "user-1").Return(&types.OrganizationMember{ID: "mem-1", OrganizationID: "org-1", UserID: "user-1", Role: "member"}, nil).Once() + repo.On("Update", mock.Anything, mock.MatchedBy(func(org *types.Organization) bool { + return org != nil && org.ID == "org-1" && org.Logo == nil + })).Return(&types.Organization{ID: "org-1", OwnerID: "user-1", Name: "Acme", Slug: "acme", Logo: nil}, nil).Once() + }, + }, } for _, tt := range tests { diff --git a/plugins/organizations/types/api.go b/plugins/organizations/types/api.go index d10659c9..0105a26c 100644 --- a/plugins/organizations/types/api.go +++ b/plugins/organizations/types/api.go @@ -6,6 +6,7 @@ import ( coreerrors "github.com/Authula/authula/core/errors" "github.com/Authula/authula/core/pagination" + coretypes "github.com/Authula/authula/core/types" "github.com/Authula/authula/models" ) @@ -141,10 +142,10 @@ func (r *CreateOrganizationRequest) Validate() error { } type UpdateOrganizationRequest struct { - Name *string `json:"name,omitempty" nullable:"true"` - Slug *string `json:"slug,omitempty" nullable:"true"` - Logo *string `json:"logo,omitempty" nullable:"true"` - Metadata map[string]any `json:"metadata,omitempty" nullable:"true"` + Name *string `json:"name,omitempty" nullable:"true"` + Slug *string `json:"slug,omitempty" nullable:"true"` + Logo coretypes.Optional[*string] `json:"logo,omitzero" nullable:"true"` + Metadata map[string]any `json:"metadata,omitempty" nullable:"true"` } func (r *UpdateOrganizationRequest) Validate() error { @@ -159,9 +160,9 @@ func (r *UpdateOrganizationRequest) Validate() error { value := strings.TrimSpace(*r.Slug) r.Slug = &value } - if r.Logo != nil { - value := strings.TrimSpace(*r.Logo) - r.Logo = &value + if r.Logo.Present && r.Logo.Value != nil { + value := strings.TrimSpace(*r.Logo.Value) + r.Logo.Value = &value } return nil }