Page MenuHomePhabricator

Add logic in AphrontDefaultApplicationConfiguration to check for and parse application/json Content-Type
Closed, InvalidPublic

Description

I am creating a Phabricator application that acts as a callback endpoint for GitHub (and perhaps other) PubSub events.
The starting code is here: https://github.com/christopher-johnson/phabricator-extensions-pubsub

The way that it works is that a webhook is created in GitHub that subscribes a Phabricator project with a callback URL. The JSON formatted event payload is routed to a phabricator url with a project id (i.e.. http://phab09.wmflabs.org/pubsub/event/1).

The problem is that the PhutilQueryStringParser is mangling the JSON before it is saved in the requestData variable. I have fixed this with a few line change to AphrontDefaultApplicationConfiguration lines 32-43.

$jsonparser = new PhutilJSONParser();
$content_type = idx($_SERVER, 'CONTENT_TYPE');
$is_form_data = preg_match('@^multipart/form-data@i', $content_type);
$is_json_data = preg_match('@^application/json@i', $content_type);
$raw_input = PhabricatorStartup::getRawInput();
if (strlen($raw_input) && !$is_form_data && !$is_json_data) {
      $data += $parser->parseQueryString($raw_input);
    } else if ($is_json_data) {
      $data += $jsonparser->parse(urldecode($raw_input));
    } else if ($_POST) {
      $data += $_POST;
}

I am not sure that this is the best fix for this problem, but it really helps me with this use case. Please advise on how to proceed.

Event Timeline

ChristopherHJohnson raised the priority of this task from to Needs Triage.
ChristopherHJohnson updated the task description. (Show Details)
ChristopherHJohnson added a project: Aphront.
epriestley claimed this task.
epriestley added a subscriber: epriestley.

Sorry, we do not support third-party application development. See T5447 for discussion.

See also:

https://secure.phabricator.com/book/phabcontrib/article/bug_reports/#supported-issues

Particularly:

We do NOT support custom code development or third-party libraries. I

I am not asking for "support" per se, so I am not sure the referenced link is applicable for this task. The use case of application/json requests is major, so rejecting it outright as some kind of non-essential fringe "custom code" seems rather strong. The problem is pretty simple and the fix even more so. I am asking you to consider the benefits of parsing a specific content-type request with an appropriate parser. I totally understand that you have priorities, etc., but this is the situation with my team. We have code on GitHub and associated build tools that will probably never move to Phabricator. How can I not consider the need to integrate these "third-party" tools seriously? I am open to suggestions. Worst case scenario is that we maintain a patched version of this class, which for such a trivial change seems ridiculous.

I am not asking for "support" per se, so I am not sure the referenced link is applicable for this task.

Ah, sorry. The intent of the documentation is to make it clear that we will not answer any questions at all about developing third-party or custom code. We simply don't have the bandwidth to do this for free when anyone asks without suffering a huge reduction in development speed. Are there changes we could make to the documentation that would make it more clear that this kind of request is not one we'll respond to?

A lot of users have questions about writing custom code, and all of them believe that their use case is very important. These come in many forms, from "how do I write X?" to "can you change Y so the X I wrote works?" (as here). We can not use the self-reported importance of a use case to prioritize third-party development, because everyone asserts that their problem is very important.

Even if we strictly have a provable, unambiguous bug in the upstream, we won't answer questions about it or fix it if there's no reproduction case in the upstream.

We do offer broad-spectrum consulting services, including help with developing third-party code, at a rate of $1,500/hr. See Consulting if this is something you're interested in. This is partly a concrete way to distinguish between actually-important requests and not-really-that-important requests.

Assuming you're working under WMF, we have some existing agreements under Paid Prioritization and might be able to work with @qgil to get a solution here prioritized, but it would be in the form of solving a larger problem in a general way, not adding a hack for your specific use case. The planned way forward for all webhook/firehose/scraping imports is through Nuance, so T8783 would probably be a blocker and some version of T3179 would likely be the ultimate upstream change. See the "Prioritization" page for details about why you can't prioritize small patches like this.

@ChristopherHJohnson the problem is that it isn't "just one change", we get requests in this vein several times a day and without a clear line on what "free support" looks like, the Phabricator project does not move forward. We just aren't staffed to manage these requests in any sort of reasonable timeframe. "Wishlist" items at best are 2-3 years out. We'd rather be honest about what the upstream can and cannot do in a reasonable timeframe so you have time to explore alternatives.

Thank you for the response.

I had not seen Nuance until you mentioned it, and I am thoroughly impressed with the design. Since NuanceSourceDefinition is still using AphrontRequest, I am pretty sure that you will need to handle application/json payloads in order for this to be useful for anything other than multipart/form data. So, in any case, this is a problem that affects (or will affect) your code base.

I will gladly create a patch for review (since a little "hack" is all that it takes for this task!). We are a non-profit organization, so paying your hourly consulting fee is just not feasible in this case, unfortunately. I really do appreciate the quality and caliber of your work and trust that your thinking process is valid. Waiting years to fix a problem (not a "wish"...) is nonetheless not a rational approach to "taking care of business".