Skip to content
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

AfterSaveBehavior default is actually .Ignore, not .Throw #2371

Closed
wants to merge 4 commits into from

Conversation

dbrownems
Copy link

Updating docs to match actual behavior of AfterSaveBehavior, and to clarify treatment of database-side changes in case of .Save and .Ignore

Updating docs to match actual behavior of `AfterSaveBehavior`, and to clarify treatment of database-side changes in case of .Save and .Ignore
@Rick-Anderson
Copy link
Contributor

@dbrownems can you check my changes? Avoid future tense and split up long sentences.

@ajcvickers
Copy link
Contributor

@Rick-Anderson The reason this isn't merged is that I still need to investigate the technical aspects of this.

@ajcvickers ajcvickers self-assigned this Jul 28, 2020
@smitpatel
Copy link
Contributor

@ajcvickers - Also include changes from #2372 depending on the decision.

@dnfadmin
Copy link

dnfadmin commented Jul 29, 2020

CLA assistant check
All CLA requirements met.

@Rick-Anderson
Copy link
Contributor

@ajcvickers - Also include changes from #2372 depending on the decision.

@smitpatel I added it.

Copy link
Author

@dbrownems dbrownems left a comment

Choose a reason for hiding this comment

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

looks good

@Rick-Anderson
Copy link
Contributor

@dbrownems thanks for the review. When I added your other commit to this PR it pushed you over the limit to require a signed CLA to accept the PR. Would you mind signing that?

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ dbrownems sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@ajcvickers
Copy link
Contributor

ajcvickers commented Jan 17, 2021

@roji Is this obsolete now that you updated these pages? Also, did you check the behavior to make sure the docs are correct?

@roji
Copy link
Member

roji commented Jan 18, 2021

Yeah, this has been corrected in #2998 - one needs to do SetAfterSaveBehavior(PropertySaveBehavior.Save) in order to provide explicit values for properties configured as OnAddOrUpdate. The default is Ignore for OnUpdate properties, which makes explicit values ignored, whereas Save makes them properly sent to the database. I've just tested this again and it works this way.

  • I'm new to this area, but I wonder - is there any reason for Ignore to be the default for on-update-generated properties? Doesn't this just make them non-overridable by default, is there any reason not to return Save here instead?
  • FWIW the XML docs on SetAfterSaveBehavior seem unclear on all this - they discuss what happens "whether this property can be modified after the entity is saved to the database", but at least in this case it's more about whether the user-set value is sent to the database or not.
  • Opened Can't add explicit value when adding for a property with AfterSaveBehavior=Save efcore#23912 for what seems to be a bug when using explicit values when adding the entity.

@roji roji closed this Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants