Page MenuHomePhabricator

repository.query is not returning required parameter `order`
Closed, ResolvedPublic

Description

Go to -> https://secure.phabricator.com/api/conduit.query

"repository.query": {
  "description": "Query repositories.",
  "params": {
    "ids": "optional list<int>",
    "phids": "optional list<phid>",
    "callsigns": "optional list<string>",
    "vcsTypes": "optional list<string>",
    "remoteURIs": "optional list<string>",
    "uuids": "optional list<string>",
    "order": "order",
    "before": "optional string",
    "after": "optional string",
    "limit": "optional int (default = 100)"
  },
  "return": "list<dict>"
},
$ echo '{ "callsigns": [ "P" ] }' | arc call-conduit --conduit-uri https://secure.phabricator.com/  repository.query
{"error":null,"errorMessage":null,"response":[{"id":"5","name":"Phabricator","phid":"PHID-REPO-9a38b7db1b795ae3e348","callsign":"P","monogram":"rP","vcs":"git","uri":"https:\/\/secure.phabricator.com\/diffusion\/P\/","remoteURI":"git:\/\/github.com\/facebook\/phabricator.git","description":null,"isActive":true,"isHosted":true,"isImporting":false,"encoding":null,"staging":{"supported":true,"prefix":"phabricator","uri":"ssh:\/\/dweller@secure.phabricator.com\/diffusion\/STAGING\/staging.git"}}]}

And there is no order field. This break api clients that do auto-discovery and validation .

(In totally unrelated news remoteURI looks like it still points at facebook's github org.)

Event Timeline

cburroughs renamed this task from repository.query is not returning required paramter `order` to repository.query is not returning required parameter `order`.
cburroughs raised the priority of this task from to Needs Triage.
cburroughs updated the task description. (Show Details)
cburroughs added a project: Conduit.
cburroughs added a subscriber: cburroughs.

This parameter is not required, although the hint should probably say "optional order".

Is the issue that the client you're using is throwing a local error like "you didn't specify parameter X, but the auto-discovery spec says it's required"?

If so, I'd generally claim this is an issue with the client you're using: I think clients generally should not do parameter validation themselves. They will always be less able to do validation than the server is, even though server validation is not especially sophisticated right now. If/when we have a formal Python API client, it is very unlikely to perform client validation. Does client validation provide any advantage?

It seems to me like you get feedback about source errors 100ms faster with client validation, at the cost of valid calls sometimes not working. Doesn't seem like a great tradeoff?

(As a philosophical point, it would also be unlikely to use conduit.query, but that's neither here nor there.)

The client is doing validation of the response including presence of fields and their type.

for k in resource.get('required', []):
    if k not in [x.split(':')[0] for x in kwargs.keys()]:
        raise ValueError('Missing required argument: %s' % k)
    if isinstance(kwargs.get(k), list) and not isinstance(resource['required'][k], list):
        raise ValueError('Wrong argument type: %s is not a list' % k)
    elif not validate_kwarg(kwargs.get(k), resource['required'][k]):
        if isinstance(resource['required'][k], list):
            raise ValueError('Wrong arguemnt type: %s is not a list of %ss' % (k, resource['required'][k][0]))
        raise ValueError('Wrong arguemnt type: %s is not a %s' % (k, resource['required'][k]))

yeah that does not make sense and would cause nothing to work I'm pretty sure

I'll add optional for clarity but in general I'd recommend, like, deleting that "arguemnt" validation since I don't think it's desirable from an architectural standpoint.

If so, I'd generally claim this is an issue with the client you're using: I think clients generally should not do parameter validation themselves. They will always be less able to do validation than the server is, even though server validation is not especially sophisticated right now. If/when we have a formal Python API client, it is very unlikely to perform client validation. Does client validation provide any advantage?

I think this may still be desirable because it eliminates a network request for a clearly invalid request. In a "properly implemented" system, the IO would be asynchronous/threaded so this initial verification would also provide the benefit of getting a sync exception.

Are you guys still interested in an "official" Python client? If so why not reuse the code we already have and rely on? I'd love to collaborate on this.