Page MenuHomePhabricator

Improve ApplicationTransaction behavior for poorly constructed transactions
ClosedPublic

Authored by epriestley on Mar 5 2014, 1:52 AM.
Tags
None
Referenced Files
F14361549: D8401.id19967.diff
Fri, Dec 20, 11:33 AM
Unknown Object (File)
Wed, Dec 4, 12:44 PM
Unknown Object (File)
Tue, Dec 3, 8:41 PM
Unknown Object (File)
Sat, Nov 30, 12:16 PM
Unknown Object (File)
Wed, Nov 27, 12:26 AM
Unknown Object (File)
Wed, Nov 27, 12:26 AM
Unknown Object (File)
Wed, Nov 27, 12:26 AM
Unknown Object (File)
Mon, Nov 25, 12:48 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