Page MenuHomePhabricator

Check for null, strictly, in maniphest.update param validation
ClosedPublic

Authored by xela on Mar 4 2014, 1:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 2:27 AM
Unknown Object (File)
Tue, Dec 3, 9:57 PM
Unknown Object (File)
Wed, Nov 27, 6:51 AM
Unknown Object (File)
Wed, Nov 27, 12:24 AM
Unknown Object (File)
Wed, Nov 27, 12:24 AM
Unknown Object (File)
Wed, Nov 27, 12:24 AM
Unknown Object (File)
Wed, Nov 27, 12:24 AM
Unknown Object (File)
Tue, Nov 26, 9:58 PM

Details

Summary

If the first non-null entry in the params array is falsey, the request bombs.

Something like {"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-on3xxsnaljmfn36d4b7a"}

Test Plan

Before:

echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
{"error":"ERR-NO-EFFECT","errorMessage":"ERR-NO-EFFECT: Update has no effect.","response":null}

After:

echo '{"id":279,"projectPHIDs":[],"status":"0","ownerPHID":"PHID-USER-cj3cpuh7oorbmnn2pl5g"}' | arc call-conduit maniphest.update
{"error":null,"errorMessage":null,"response":{"id":"279","phid":"PHID-TASK-lbwcq3pmur2c5fuqqhlx"...

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Skipped
Unit
No Test Coverage

Event Timeline

xela updated this revision to Unknown Object (????).Mar 4 2014, 1:39 AM

figured out why xhpast wasn't working on stock ec-2, so I can lint like I should

sudo yum install gcc-c++
xela edited edge metadata.
epriestley edited edge metadata.

I'm going to pull this since it's clearly an improvement. In the long run, I think things will look something like this:

  • We'll expose a low-level app.applytransactions method to all ApplicationTransactions applications (which, increasingly, is "all applications which do anything interesting"). This will be more difficult to use, but give you full control over all possible transaction types and fine-grained validation, error handling, etc.
  • We'll retain most of these app.update/app.comment-style single-purpose APIs, but they'll become high-level convenience wrappers for app.applytransactions.

ApplicationTransactions includes sophisticated detection of no-effect updates, including updates which have no effect because they set a value to the current value (e.g., changing status from "Open" to "Open"). This is the real code we should be using to determine that an input has no effect. But we can cross this bridge when we get there.

This revision is now accepted and ready to land.Mar 4 2014, 7:42 PM

Closed by commit rP5fabda2a6db1 (authored by @xela, committed by @epriestley).