Page MenuHomePhabricator

Conduit accept int/bool parameters as strings
ClosedPublic

Authored by gd on Oct 12 2016, 1:32 PM.

Details

Summary

Accept Conduit parameter values as strings (e.g. from curl) and convert to required type.

Test Plan

Call conduit method with int/bool parameter iusing curl and make sure it does not result in validation error, e.g.

$ curl http://$PHABRICATOR_HOST/api/maniphest.search -d api.token=$CONDUIT_TOKEN -d constraints[modifiedEnd]=$(date +%s) -d constraints[hasParents]=true -d limit=1

Fixes T10456.

Diff Detail

Repository
rP Phabricator
Branch
conduit-accept-strings
Lint
Lint OK
Unit
Unit Tests OK
Build Status
Buildable 14080
Build 18267: arc lint + arc unit

Event Timeline

gd retitled this revision from to Conduit accept int/bool parameters as strings.
gd updated this object.
gd edited the test plan for this revision. (Show Details)
gd added a reviewer: epriestley.
gd edited edge metadata.
  • s/validateStringList/parseStringList/ to be consistent with other parse<Type>() methods

I think it is a good idea to indicate that we may be returning entirely different value as opposed to just validating the passed one.

This weakens JSON parsing, right? Today, if you call Conduit with one of the "pass some JSON" mechanisms -- versus passing raw HTTP parameters -- I think this produces a type error:

{
  "someBooleanParameter": "1"
}

I think that's correct, and that we should retain that behavior. If you're passing a boolean value via a protocol that supports JSON, you should pass an actual boolean.

I think this change will weaken this behavior, so that "1"will now be interpreted as true, even when it is appearing in JSON.

Instead, I think we should apply these more-liberal rules only when the parameters are coming in as raw HTTP parameters, so ?someBooleanParameter=1&... is fine, but "someBooleanParameter": "1" is a type error.

The existing HTTPParameter classes already have logic for liberal type interpretation, so it may make sense to reuse them. Roughly, I imagine something like this:

  • Instead of always taking the same pathway through ConduitParameterType, we add some slightly different pathway when the inbound request is raw HTTP.
  • That pathway applies the more liberal casts (possibly by invoking HTTPParameterType logic?) first, then runs through the normal logic?

The total number of types is fairly small and I don't expect them to expand much, so code reuse is probably not hugely important -- e.g., if trying to use AphrontBoolHTTPParameterType to parse boolean values is cumbersome, just duplicating the logic seems fine. It probably differs in some cases anyway (e.g., we probably should not accept comma-separated string lists for Conduit, but should continue to do so for manually-entered HTTP requests.

Upshot:

  • Don't weaken type validation logic when receiving JSON.
  • Instead, add a "liberal validation" pathway for Conduit calls where the parameters are raw HTTP parameters.
  • Reusing logic from HTTPParameterType classes would be vaguely nice if it isn't a mess, but it probably is a mess.
gd edited edge metadata.
  • Relaxed Conduit parameter parsing for non-JSON requests only

HTTPParameterType and ConduitParameterType seems to be more tied to the request source (e.g. web form or conduit) then request data type. Some fields for example are conduit-only and do not even return HTTPParameterType.

The knowledge of data type was lost in controller so what I tried to do is basically take it and pass it all the way down to ConduitParamterType where actual knowledge of expected data types is and where data validation/parsing happens.

epriestley edited edge metadata.

This seems reasonable to me, thanks!

This revision is now accepted and ready to land.Oct 14 2016, 1:23 PM
This revision was automatically updated to reflect the committed changes.