Page MenuHomePhabricator

Improve ApplicationTransaction behavior for poorly constructed transactions
ClosedPublic

Authored by epriestley on Mar 5 2014, 1:52 AM.
Tags
None
Referenced Files
F14056862: D8401.diff
Sat, Nov 16, 10:52 PM
F13992288: D8401.id19955.diff
Tue, Oct 22, 3:38 PM
F13981413: D8401.diff
Oct 19 2024, 4:22 PM
F13975727: D8401.id.diff
Oct 18 2024, 11:23 AM
F13968032: D8401.id.diff
Oct 16 2024, 6:05 PM
F13965467: D8401.id.diff
Oct 16 2024, 2:18 AM
F13962924: D8401.id.diff
Oct 15 2024, 1:00 PM
Unknown Object (File)
Oct 13 2024, 6:36 PM
Subscribers

Details

Summary

Ref T2222. Five very small improvements:

  • I hit this exception and it took a bit to understand which transaction was causing problems. Add an Exception subclass which does a better job of making the message debuggable.
  • The oldValue of a transaction may be null, legitimately (for example, changing the repositoryPHID for a revision from null to some valid PHID). Do a check to see if setOldValue() has been called, instead of a check for a null value.
  • Add an additional check for the other case (shouldn't have a value, but does).
  • When we're not generating a value, don't bother calling the code to generate it. The best case scenario is that it has no effect; any effect it might have (changing the value) is always wrong.
  • Maniphest didn't fall back to the parent correctly when computing this flag, so it got the wrong result for CustomField transactions.
Test Plan

Resolved the issue I was hitting more easily, made updates to a null-valued custom field, and applied other normal sorts of transactions successfully.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

btrahan edited edge metadata.

Nice. Thanks for the followup!

This revision is now accepted and ready to land.Mar 5 2014, 6:33 PM