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

Database integrity fixes #243

Merged
merged 8 commits into from
Feb 20, 2019
Merged

Conversation

morganbr
Copy link
Contributor

Fix various database integrity issues that would either allow corruption or prevent deleting entities. Changes include:

  • Adding [Required] to foreign key properties where we want cascading deletes (or where it would be bad if the value was ever null)
  • SQL can't handle cases where there could be multiple deletion cascading paths from one entity to another (for example, from Event to both Puzzle and Team and then from both of those to PuzzleStatePerTeam). In those cases, this change removes one of the cascade paths and deletes the child entity manually instead.
  • Dealing with input validation failures due to missing [Required] properties. This typically happens on Create or Edit pages where we set those values after the user submits it or where we don't want to round-trip it through editing. ModelState.Remove lets us remove validation for only the fields we know won't matter.
  • Fixing some cases where we weren't setting required properties
  • Miscellaneous minor bug fixes found while testing everything

Fixes #177, fixes #106, fixes #59, fixes #104

@morganbr morganbr self-assigned this Feb 14, 2019
@morganbr morganbr requested review from asyasky and tabascq February 14, 2019 05:40
Copy link
Contributor

@tabascq tabascq 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 to me! Only one question, which I'm sure is the right thing to do, but since I will certainly need to do it from time to time, I'd like to understand better.

@@ -31,6 +31,7 @@ public IActionResult OnGet()

public async Task<IActionResult> OnPostAsync()
{
ModelState.Remove("Puzzle.Event");
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it in many places but its meaning isn't clear to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It tells the page not to validate the Event property -- as submitted, it's invalid because it's null (and thus a violation of [Required]. We set it further down before sending it to the database. There are some similar cases on Edit pages where we just don't roundtrip a required property (usually the DB just don't get changed for those).

@morganbr morganbr merged commit 5fd026f into PuzzleServer:master Feb 20, 2019
@morganbr morganbr deleted the DBIntegrity branch February 20, 2019 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants