-
Notifications
You must be signed in to change notification settings - Fork 26
feat: Compatible with product bundle #113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
fe24be0 to
54eaaea
Compare
54eaaea to
fc9865c
Compare
MalteJanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reaching out and taking care of this change 👍 . But I think we need to discuss a little further about the chosen approach and figure out a more robust one 🙂
| // Check if product already exists (was previously migrated) before creating mapping | ||
| // This is needed because the 'type' field has Immutable flag and can only be set on create | ||
| $existingMapping = $this->mappingService->getMapping( | ||
| $this->connectionId, | ||
| DefaultEntities::PRODUCT, | ||
| $data['id'], | ||
| $this->context | ||
| ); | ||
| $isUpdate = $existingMapping !== null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first look, this is a clever attempt to determine if this entity will run into an insert or update. But unfortunately it contains a flaw 🙈 .
Yes on the first migration run this will tell you if the product "was previously attempted to migrate" and thus give the the wanted result.
But if the migration for this entity somehow fails:
- either further down in this converter
- in the validation layer that comes after - which we currently implement in our feature branch
- or later while writing the entity to the corresponding SW6 table
it would mean the next migration run (retry) would still consider this entity as an "update", because the mapping already exists, which produces the wrong outcome in this case.
We have to think further about this, to not introduce yet another N+1 Query performance issue 😬
| $converted = $data; | ||
|
|
||
| // Check if custom field set already exists (was previously migrated) before creating mapping | ||
| // This is needed because the 'name' field has Immutable flag and can only be set on create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not raising this concern earlier:
Do you really think it's a good idea to have such a ImmutableFlag ? (Was introduced in this PR )
Because I already see the extra work it creates for us here, every entity that uses that flag needs an extra special treatment in the migration and if someone adds that flag to other fields in the future (or introduces such fields), we will run into issues again.
It basically makes "upserts" (insert / update) really difficult to do for such entities, because you want to create them if they don't exists yet, or update them otherwise, so you need the field to be defined but also not defined at the same time... 🤯
I also can't imagine that it's nice to work with such an API, e.g. imagine:
- You read the data of an existing entity over our admin API
- You modify some value
- You want to update that entity with the same payload
I think this was possible in the past (correct me if I'm wrong), but now you need to remove certain fields from your payload to send the update request, which is really annoying.
Of course in our administration codebase this is a non-issue, because there we use some extra logic / magic to build a changeset diff and only send that back to the server, but do you really expect every other API client to do the same or treat it differently like we do here in the migration? 😬
I would suggest either:
- Allowing updates in the DAL when the value of the
Immutablefield stays the same (is unchanged), as it should be in these cases like here in the "upsert" case for migration. Otherwise it would be a valid error if the migration tries to change such a value - Or "silently" ignore that field in the DAL update case, without throwing an error. Maybe it should still create a log entry though 🤷♂️
I'm open for other suggestions as well, but I think we should have a better answer for the "upsert" case than just deal with it by throwing extra code at it. The Migration-Assistant makes heave use of "upsert" and basically writes all it's data that way:
| $writeResults = $this->entityWriter->upsert( |
In this PR: