shopware icon indicating copy to clipboard operation
shopware copied to clipboard

feat: introduce immutable flag

Open vienthuong opened this issue 2 weeks ago • 2 comments

There are many cases that we want a field to be only set on creating but not on updating, but currently we don't have any flag that prevent updating that field. The WriteProtected flag will also prevent the field from creating so it's not ideal

Example usecases is custom field type and name, where the fields are disabled on UI but nothing prevent it from being updated via admin api.

The integration tests should show its usecases

image image

vienthuong avatar Dec 10 '25 16:12 vienthuong

OpenAPI Snapshot

Project: shopware/store-api

No changes detected

Your OpenAPI schema is identical to the base branch (trunk).

Links:
All Project Snapshots · Base Branch Snapshot

Project: shopware/admin-api

No changes detected

Your OpenAPI schema is identical to the base branch (trunk).

Links:
All Project Snapshots · Base Branch Snapshot

explore-openapi[bot] avatar Dec 10 '25 16:12 explore-openapi[bot]

Codecov Report

:x: Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 54.21%. Comparing base (99a5f9c) to head (0dee2eb). :white_check_mark: All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...k/App/Lifecycle/Persister/CustomFieldPersister.php 0.00% 4 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #13957      +/-   ##
==========================================
+ Coverage   54.14%   54.21%   +0.06%     
==========================================
  Files        3204     3204              
  Lines       96417    96448      +31     
==========================================
+ Hits        52204    52287      +83     
+ Misses      44213    44161      -52     
Flag Coverage Δ
phpunit-migration 92.92% <ø> (ø)
phpunit-unit 51.92% <87.87%> (+0.06%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Dec 10 '25 16:12 codecov[bot]

Add the docs for the new flag here https://github.com/shopware/docs/pull/2040 and also in the flag class itself

@keulinho It's possible to omit the immutable fields behind the scene when the request is a patch, but I am not sure if it surprises clients that they send the attribute in the payload, but the field is not updated, even though the request was successful. Is it something concerning for you @patzick

About Open API Schema, I found the related discussion here, but it seems it's currently not supported, not sure if there's any alternative to make it clearer for clients

https://github.com/OAI/OpenAPI-Specification/issues/2720

vienthuong avatar Dec 12 '25 04:12 vienthuong

@keulinho @mitelg As you can see in this commit https://github.com/shopware/shopware/pull/13957/commits/161353ee257eec340ca9b445b4c1d7e231d5f735, I unset the name of the custom field set provided by the app manifest when the set already exists, which should make sense that they are not allowed to change the name of the custom_field_set and custom_fields, but I'm not sure if it will upset someone 😅 Is it considered a breaking change? If it is, I can add a Feature flag check in the definition to make it safe

vienthuong avatar Dec 12 '25 04:12 vienthuong

@vienthuong afaik the custom field sets and custom fields are identified via their name anyway. so if they would change, it would result in a new custom field (set), wouldn't it?

mitelg avatar Dec 12 '25 07:12 mitelg

@mitelg ah seems it's the case \Shopware\Core\Framework\App\Lifecycle\Persister\CustomFieldPersister::upsertCustomFieldSets

Then it'd be fine, the upsert set the current name to the payload even when the name doesn't change

vienthuong avatar Dec 12 '25 07:12 vienthuong

yes, I don't think, we need to change something there :+1:

mitelg avatar Dec 12 '25 08:12 mitelg

ah no, we would need to remove the name from the payload, otherwise the update command will fail, because of the flag, right? until now, we don't have a check, if the name is different or not, right? not sure, if we want to make an exception here and basically allow the update case if the value is exact the same :thinking:

mitelg avatar Dec 12 '25 08:12 mitelg

Yes, I mean we don't need to add a major flag check but we need to unset the name in the payload, which is what I did in https://github.com/shopware/shopware/commit/161353ee257eec340ca9b445b4c1d7e231d5f735

until now, we don't have a check, if the name is different or not, right? not sure, if we want to make an exception here and basically allow the update case if the value is exact the same 🤔

Would be more convenient to have that, but we would need an additional query to check if the value doesn't change so it'd also not performant 😅

vienthuong avatar Dec 12 '25 08:12 vienthuong

That's a great improvement and I agree with @keulinho that it should be reflected in the schema. Immutable fields should never appear on the schema of update endpoint.

As a general generation rule we should have something like EntityBase, EntityCreate, EntityUpdate definitions. We should declare them either as separate components or directly inside endpoints if it's not repeated.

So assuming name is our immutable field and description we can modify always we should have it declared in:

  • EntityUpdate - description, not required
  • EntityCreate - all fields that this entity have (without technical ones like id, createdAt)
  • MyEntity - all fields that are returned by the API (with technical like id, createdAt...)

Small example I prepared for that, interactive version where you can see previews

Important parts:

Entities example ```json "components": { "schemas": { "BaseEntity": { "type": "object", "description": "Base entity with technical details", "required": [ "id", "createdAt" ], "properties": { "id": { "type": "string", "readOnly": true, "description": "Server-generated unique ID" }, "createdAt": { "type": "string", "format": "date-time", "readOnly": true } } }, "MyEntityUpdate": { "type": "object", "description": "Fields that are available (or required) for the update", "required": [ "description" ], "properties": { "description": { "type": [ "string", "null" ] } } }, "MyEntityCreate": { "allOf": [ { "$ref": "#/components/schemas/MyEntityUpdate" }, { "type": "object", "properties": { "name": { "type": "string", "description": "Editable only on creation. Immutable afterwards." } } } ], "required": [ "name" ] }, "MyEntity": { "description": "All unique fields we have (apart of technical ones) in entity.", "allOf": [ { "$ref": "#/components/schemas/BaseEntity" }, { "$ref": "#/components/schemas/MyEntityCreate" } ] } } } ```
Endpoints example ```json "paths": { "/entities": { "post": { "summary": "Create a new MyEntity", "operationId": "createMyEntitity", "requestBody": { "required": true, "content": { "application/json": { "schema": { "$ref": "#/components/schemas/MyEntityCreate" } } } }, "responses": { "201": { "description": "Created MyEntity", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/MyEntity" } } } }, "400": { "description": "Bad request" } } }, "get": { "summary": "List all MyEntities", "operationId": "getMyEntities", "responses": { "200": { "description": "List of MyEntities", "content": { "application/json": { "schema": { "type": "array", "items": { "$ref": "#/components/schemas/MyEntity" } } } } }, "400": { "description": "Bad request" } } } }, "/entities/{id}": { "get": { "summary": "Get a single MyEntity", "operationId": "getMyEntity", "parameters": [ { "name": "id", "in": "path", "required": true, "schema": { "type": "string" } } ], "responses": { "200": { "description": "MyEntity object", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/MyEntity" } } } }, "400": { "description": "Bad request" } } }, "patch": { "summary": "Update a MyEntity", "operationId": "updateMyEntity", "parameters": [ { "name": "id", "in": "path", "required": true, "schema": { "type": "string" } } ], "requestBody": { "required": true, "content": { "application/json": { "schema": { "$ref": "#/components/schemas/MyEntityUpdate" } } } }, "responses": { "200": { "description": "Updated MyEntity", "content": { "application/json": { "schema": { "$ref": "#/components/schemas/MyEntity" } } } }, "400": { "description": "Attempted to modify immutable fields" } } } } }, ```

patzick avatar Dec 15 '25 09:12 patzick

Created follow-up issue to improve schema generation also taking into consideration immutable flag: #14039

patzick avatar Dec 16 '25 09:12 patzick