Skip to content

Fix: sync DB-generated values back to original models in CrudService.SaveChangesAsync#2988

Open
alexeyshibanov wants to merge 4 commits intodevfrom
fix/crud-service-sync-model-after-save
Open

Fix: sync DB-generated values back to original models in CrudService.SaveChangesAsync#2988
alexeyshibanov wants to merge 4 commits intodevfrom
fix/crud-service-sync-model-after-save

Conversation

@alexeyshibanov
Copy link
Contributor

@alexeyshibanov alexeyshibanov commented Mar 9, 2026

Summary

  • CrudService.SaveChangesAsync replaces changedEntry.NewEntry with a new model created via parameterless entity.ToModel() extension. This means the original model objects passed to SaveChangesAsync never receive DB-generated values (auto-incremented IDs, foreign keys, etc.)
  • This fix adds an optional model parameter to the virtual ToModel method: ToModel(TEntity entity, TModel model = null). When model is provided, it updates in-place via entity.ToModel(model); when null, it creates a new instance (preserving existing behavior)
  • The post-save loop now calls changedEntry.NewEntry = ToModel(changedEntities[i], changedEntry.NewEntry), routing through the virtual method so that downstream overrides (e.g. PaymentMethodsService.ToModel) are respected

Breaking change

The ToModel signature changed from ToModel(TEntity entity) to ToModel(TEntity entity, TModel model = null). Existing overrides will get a compile error and must update their signature. This is intentional — a compile error is better than a silent regression where the override stops being called.

Known overrides to update:

Motivation

Modules that need DB-generated values synced back to the caller's model objects are forced to copy-paste the entire SaveChangesAsync method just to change this single line. CustomerOrderService already does this. ShoppingCartService needs it too for the same reason.

Fixing at the CrudService level eliminates the need for per-module workarounds and ensures consistent behavior across all CRUD services.

Test plan

  • Existing CrudServiceTests pass (7/7)
  • Verify no regressions in module integration tests

🤖 Generated with Claude Code
Image tag:
ghcr.io/VirtoCommerce/platform:3.1008.0-pr-2988-bcae-crud-service-sync-model-after-save-bcae5eb5


Note

Medium Risk
Touches the generic CrudService used across modules and changes model-mapping flow after persistence; reflection-based override detection and in-place updates could affect downstream custom ToModel overrides and event payloads.

Overview
Fixes CrudService.SaveChangesAsync to sync database-generated fields back into the same model instances (e.g., IDs/keys) by mapping entities into the existing changedEntry.NewEntry instead of always creating a new model.

Introduces a new ToModel(TEntity entity, TModel model) overload that can update in-place, marks the old ToModel(TEntity) as obsolete, and updates read/save code paths to call the new overload while preserving legacy overrides via runtime override detection.

Written by Cursor Bugbot for commit bcae5eb. This will update automatically on new commits. Configure here.

…SaveChangesAsync

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@vc-ci
Copy link
Contributor

vc-ci commented Mar 9, 2026

Review task created: https://virtocommerce.atlassian.net/browse/VCST-4749

Copy link
Contributor

@vc-ci vc-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test Suite: Test Suites/Modules/module_Assets
Tests: 13
Failures: 0
Errors: 0
Time: 7.975
Timestamp: 09-03-2026T14:29:29

// Update the original model in place instead of creating a new one.
// This ensures DB-generated values are synced back to the original model objects
// passed to SaveChangesAsync.
changedEntities[i].ToModel(changedEntry.NewEntry);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Fixed — the loop now calls the virtual ToModel with a new optional parameter:

changedEntry.NewEntry = ToModel(changedEntities[i], changedEntry.NewEntry);

The ToModel signature changed to ToModel(TEntity entity, TModel model = null):

  • model = null → creates a new object via entity.ToModel() (existing behavior for GetAsync and originalModel creation)
  • model != null → in-place update via entity.ToModel(model) (new behavior for post-save sync)

This is an intentional breaking change for existing overrides like PaymentMethodsService.ToModel(entity) — they'll get a compile error and need to update their signature. A compile error is better than a silent regression where the override stops being called.

Change ToModel signature to accept optional target model parameter.
When target is provided, updates it in-place; when null, creates new
instance (preserving existing behavior for GetAsync and change tracking).

This is a breaking change for CrudService overrides of ToModel(entity) —
they must update to the new signature ToModel(entity, model).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 13, 2026

Copy link
Contributor

@vc-ci vc-ci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test Suite: Test Suites/Modules/module_Assets
Tests: 13
Failures: 0
Errors: 0
Time: 7.825
Timestamp: 13-03-2026T11:32:07

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants