Page MenuHomePhabricator
Create Task
Maniphest T11276

diffusion.uri.edit for creating an object fails depending on order of transactions in input
Closed, ResolvedPublic
Actions

Description

diffusion.uri.edit fails if uri is specified before repository:

$ echo '{
  "transactions": [
    {
      "type": "uri",
      "value": "http://home.place/some/uri.git"
    },
    {
      "type": "repository",
      "value": "PHID-REPO-7n64jo2jw3uivhysrxgr"
    },
    {
      "type": "disable",
      "value": true
    }
  ]
}' | arc call-conduit diffusion.uri.edit

Attempting to access attached data on PhabricatorRepositoryURI (via getRepository()), but the data is not actually attached

But works fine if uri happens after repository:

$ echo '{
  "transactions": [
    {
      "type": "repository",
      "value": "PHID-REPO-7n64jo2jw3uivhysrxgr"
    },
    {
      "type": "uri",
      "value": "http://home.place/some/uri.git"
    },
    {
      "type": "disable",
      "value": true
    }
  ]
}' | arc call-conduit diffusion.uri.edit
{
  "object": {
    "id": 457,
    "phid": "PHID-RURI-x7ildq64yt3nry2fbe2t"
  },
...

stacktrace:

PhabricatorLiskDAO::assertAttached called at [<phabricator>/src/applications/repository/storage/PhabricatorRepositoryURI.php:84]
PhabricatorRepositoryURI::getRepository called at [<phabricator>/src/applications/repository/storage/PhabricatorRepositoryURI.php:310]
PhabricatorRepositoryURI::getForcedProtocol called at [<phabricator>/src/applications/repository/storage/PhabricatorRepositoryURI.php:272]
PhabricatorRepositoryURI::getURIObject called at [<phabricator>/src/applications/repository/storage/PhabricatorRepositoryURI.php:219]
PhabricatorRepositoryURI::getEffectiveURI called at [<phabricator>/src/applications/diffusion/editor/DiffusionURIEditor.php:104]
DiffusionURIEditor::applyCustomInternalTransaction called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:549]
PhabricatorApplicationTransactionEditor::applyInternalEffects called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:936]
PhabricatorApplicationTransactionEditor::applyTransactions called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1806]
PhabricatorEditEngine::buildConduitResponse called at [<phabricator>/src/applications/transactions/editengine/PhabricatorEditEngineAPIMethod.php:40]
PhabricatorEditEngineAPIMethod::execute called at [<phabricator>/src/applications/conduit/method/ConduitAPIMethod.php:122]
ConduitAPIMethod::executeMethod called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:131]
ConduitCall::executeMethod called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:81]
ConduitCall::execute called at [<phabricator>/src/applications/conduit/controller/PhabricatorConduitAPIController.php:81]
phlog called at [<phabricator>/src/applications/conduit/controller/PhabricatorConduitAPIController.php:101]
PhabricatorConduitAPIController::handleRequest called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:237]
AphrontApplicationConfiguration::processRequest called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:149]
AphrontApplicationConfiguration::runHTTPRequest called at [<phabricator>/webroot/index.php:17]

(From darien via irc)

Revisions and Commits

rP Phabricator
D16237rP921d56efb0f0 Make repository URI creation work regardless of "repository" transaction order

Event Timeline

avivey created this task.Jul 5 2016, 8:42 PM
Herald added a subscriber: eadler. · View Herald TranscriptJul 5 2016, 8:42 PM
avivey renamed this task from diffusion.uri.edit fails depending on order of transactions in input to diffusion.uri.edit for creating an object fails depending on order of transactions in input.Jul 5 2016, 8:43 PM
danudey added a subscriber: danudey.Jul 5 2016, 8:55 PM
epriestley added a subscriber: epriestley.Jul 5 2016, 9:10 PM

My initial thought is that we should just reorder these transactions. We should use a stable sort, but that's relatively easy now that we have msortv(). There are probably a handful of other transactions with similar properties in other object types.

avivey added a comment.Jul 5 2016, 11:22 PM

That kinda feels like a band-aid... Shouldn't we be able to not-crash for any input? Like if a user will only provide a uri transaction?

epriestley added a comment.Jul 5 2016, 11:31 PM

It's impossible to create a URI without a repository transaction (it doesn't make sense, since URI objects can't just exist), and we already give you an appropriate error when you try:

epriestley@orbital ~ $  echo '{
  "transactions": [
    {
      "type": "uri",
      "value": "http://example.com"
    }
  ]
}' | arc call-conduit --conduit-uri http://local.phacility.com/ diffusion.uri.edit
{"error":"ERR-CONDUIT-CORE","errorMessage":"ERR-CONDUIT-CORE: Validation errors:\n  - When creating a repository URI, you must specify which repository the URI will belong to.","response":null}

We verify that you have a valid repository transaction, but then we try to execute the transactions in an order that doesn't work.

avivey added a comment.Jul 5 2016, 11:32 PM

ok, that makes sense.

epriestley added a comment.Jul 5 2016, 11:34 PM

A possible alternate approach would be to attachRepository() as a side-effect of validating the transaction instead of while applying the transaction, but then if something throws between the "validate" and "apply" steps we could end up with a URI in a weird state. That probably never actually matters, but feels a little more awkward to me than ordering the transactions.

Or we can do something like explicitly attaching the repository before doing this particular edit. This situation is pretty weird and none of the fixes feel super great to me.

That is, this probably fixes it:

diff --git a/src/applications/diffusion/editor/DiffusionURIEditor.php b/src/applications/diffusion/editor/DiffusionURIEditor.php
index c22b888..de5bfc3 100644
--- a/src/applications/diffusion/editor/DiffusionURIEditor.php
+++ b/src/applications/diffusion/editor/DiffusionURIEditor.php
@@ -77,6 +77,7 @@ final class DiffusionURIEditor
           $old_uri = $object->getEffectiveURI();
         } else {
           $old_uri = null;
+          $object->attachRepository($this->repository);
         }
 
         $object->setURI($xaction->getNewValue());

Maybe that's really the best fix since it's very simple, at least.

epriestley added a revision: D16237: Make repository URI creation work regardless of "repository" transaction order.Jul 5 2016, 11:40 PM
epriestley closed this task as Resolved by committing rP921d56efb0f0: Make repository URI creation work regardless of "repository" transaction order.Jul 5 2016, 11:45 PM
epriestley added a commit: rP921d56efb0f0: Make repository URI creation work regardless of "repository" transaction order.
Phabricator · Built by Phacility