feat: introduce immutable flag
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
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).
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.
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
@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 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 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
yes, I don't think, we need to change something there :+1:
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:
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 😅
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" } } } } }, ```
Created follow-up issue to improve schema generation also taking into consideration immutable flag: #14039