feat: update how we check immutability #3188
Conversation
There was a problem hiding this comment.
Is there a test that reproduces the issue you are trying to solve here, using the standard ModelInstanceDocument.replace method? That method already constructs the json-patch to only include fields that actually changed, so I'm not sure how this could ever be a problem in practice.
Yes regarding the test, I changed the model for the set test to include an immutable field. The jsonpatch construction is not the problem, its how we validate immutability on locked fields, thats why we now will compare the previous value with the value post jsonpatch. Also this is a problem we are currently having, you can check the linear task for a step by step reproduction, those steps will trigger the |
packages/stream-model-instance-handler/src/__tests__/model-instance-document-handler.test.ts
Outdated
Show resolved
Hide resolved
| const modelStream = await context.loadStream<Model>(metadata.model) | ||
| await this._validateContent(context, modelStream, newContent, false, payload) | ||
| await this._validateContent(context, modelStream, newContent, false, payload, oldContent) |
There was a problem hiding this comment.
Couldn't state.content be passed directly here, having the logic handle if it's null?
There was a problem hiding this comment.
yes it could, but the oldContent line already handles if state.content its null, what would be the benefit of doing that here?
There was a problem hiding this comment.
I just think the null value should be provided directly, no need to provide an empty object as fallback, so the following logic can do the null check.
Basically, having an empty object doesn't guaranty that the content wasn't set in the immutable fields check.
There was a problem hiding this comment.
Rather than relying on the content being null, which feels like an indirect check of what we want and maybe not entirely future-proof (what if stream content could be explicitly set to null in the future?), could we instead check that the stream has a set account relation and that this is the first data commit? That feels like the actual rule that should allow content to be set for the first time.
There was a problem hiding this comment.
you mean checking the state.log and confirm this is the first data commit? or do you mean something else?
There was a problem hiding this comment.
yeah exactly. Will also need to check the Model this MID belongs to and see if it's using set or single account relations (also I just remembered single account relation likely has the exact same problem, so we should probably add a test for it also)
packages/stream-model-instance-handler/src/model-instance-document-handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think things are correct now, but this approach introduces a lot of extra clones of the content which is not ideal. Could we go back to the original approach of checking the jsonPatch for if it touches any of the immutable fields, but just add an explicit exception for if the stream has the SET account relation and the commit being applied is the very first data event?
packages/stream-model-instance-handler/src/model-instance-document-handler.ts
Outdated
Show resolved
Hide resolved
packages/stream-model-instance-handler/src/model-instance-document-handler.ts
Outdated
Show resolved
Hide resolved
packages/stream-model-instance-handler/src/model-instance-document-handler.ts
Outdated
Show resolved
Hide resolved
JulissaDantes
deleted the
feature/ws2-3139-models-with-set-relation-and-immutable-fields
branch
Description
We used the payload data to determine if an immutable field was being altered, but this caused 2 problems:
upsertcommand will always throw if creating a new record for a model with an immutable field. e.g. ComposeDB