Page MenuHomePhabricator

Improve ApplicationTransaction behavior for poorly constructed transactions
ClosedPublic

Authored by epriestley on Mar 5 2014, 1:52 AM.
Tags
None
Referenced Files
F13166578: D8401.diff
Tue, May 7, 6:15 AM
Unknown Object (File)
Tue, Apr 30, 12:09 AM
Unknown Object (File)
Sat, Apr 27, 6:41 PM
Unknown Object (File)
Sat, Apr 27, 6:41 PM
Unknown Object (File)
Wed, Apr 24, 10:04 PM
Unknown Object (File)
Sat, Apr 20, 6:33 PM
Unknown Object (File)
Tue, Apr 16, 7:43 AM
Unknown Object (File)
Tue, Apr 9, 5:05 AM
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

Repository
rP Phabricator
Branch
statetrans1
Lint
Lint Passed
Unit
No Test Coverage

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