Re: Extensions to REST-API

On Thu, Mar 28, 2019 at 09:05:22AM -0400, John P. Rouillard wrote:
> Hi Ralf:
> 
> In message <[email protected]>,
> Ralf Schlatterbeck writes:
> >I'm currently missing some more things in the REST API before it becomes
> >really useful:
> >- The ability to look up keys of items in the database, e.g., say I want
> >  to find the status with the name 'closed'. ...
> >  The string search is a case insensitive substring search and
> >  will find stati where the name includes the string 'closed'. So if we
> >  have another status with the name 'almost closed' it will also be
> >  found (because it contains the substring 'closed').
> 
> My thought was:
> 
>   rest/data/issue?@q=title:string
> 
> replaces
> 
>   title=string
> 
> so title=string is an absolute match while @q... is the substring
> match. So rest/data/status?@q=name:closed would be a substring search
> and rest/data/status?name=closed would be the absolute name. This
> would also mean that @q would play the role of the all text search
> capability on the web interface.

Sounds nice. Needs an update to filter/filter_iter methods in the
backends.
For now I've solved that as indicated with

rest/data/status/closed
or more verbose
rest/data/status/name=closed

Note that the name= variant is only needed if the keys can be numeric
(in which case we would not be able to distinguish a key from an ID)

But absolute string matching is definitely something we should
implement before the next release. The @q parameter for the substring
searching sounds nice.

If you want to test my recent changes, be sure to update, I've pushed a
bug-fix (I had reused a variable).

Also note that I've remove a lower() for the path, this lead to keyword
searches failing if the keys included uppercase letters (path includes
url parameters which are case sensitive!)

> Speaking of all text search, I also did this:
> 
>   rest/data/msg?content=testing
> 
> I gave up after 3 minutes 8-). I think the content is being searched
> by grabbing each item and searching content without using the FTS. So
> some mechanism to tie into the full text search index is probably
> needed.

Yes, maybe just document that text matching (being it absolute or
substring) on a content prop will not work. And maybe add index searches
some time in the future.

Concerning index: I'm not very happy with the current state of affairs
of indexing binary files, as far as I understand the indexer currently
indexes everything.

> >  IMO this should be extended to allow key attributes directly in the
> >  URL, e.g. .../data/status/closed
> >  Now this would work only for classes that have a key attribute (so we
> >  can't search for issue title this way) but would be nice for finding a
> >  certain status or a user (.../data/user/example_username)
> 
> Agreed, id or key field should work. One question if closed is a
> retired status, what gets returned? I don't think 200 is correct as a
> retired state indicates it's not supposed to be used. I would claim at
> minimum 404 (not found), but 410 Gone may be better. At least 410
> indicates that a action PATCH request to restore/unretire the status
> would be accepted.

Currently this would return a 404. The retired status will not show up
in searches. Note that I implemented a retired parameter to the filter
method in hyperdb recently, so we *could* search for retired items. This
however would require two searches or iterating over several returned
items of which one may not be retired. So not something we want to do in
the general case: There can be multiple retired items for a single key
prop!

> >- The query on a class should directly yield the attributes if
> >  requested, so if we specify, say, @verbose=3 we would get all
> >  attributes (except for content) along with the query, e.g.
> >  /data/status?@verbose=3 would yield
> >  'data' : 
> >      '@total_size': '3'
> >    , 'collection':
> >    [ { 'link': ...
> >      , 'id': '1'
> >      , 'name': 'open'
> >      , 'attributes' :
> >        { 'name' : ...
> >        , ...
> >        }
> >      }
> >    , ...
> >    ]
> >  If this is not possible we're wasting a lot of roundtrip time to
> >  request each item of which we only have the id (and the label property
> >  if @verbose is >1 according to Johns latest update) in turn. Similar
> >  to the same problem in XMLRPC and in the native roundup API (which
> >  does a separate database request for each item).
> 
> I have code enabling @embed=propname:propname:propname so the client
> can request any props they want at the class level. I am testing and
> can push it (maybe to a branch) this evening but it's live at:
> 
>   https://rouilj.dyn amic-dns.net/demo/rest/data/
> 
> (just remove the space).

Sounds nice. How about @attributes instead of @embed, or is embed
something other interfaces are using?

I'm also thinking about using @verbose here similar to querying a single
item and with the same semantics as with a single item, how about:

@verbose     single item           class
0            only IDs for links    No attributes per item
1            only IDs for links    Attributes
2            id + link             Attributes with id + link
3            id + link + key       Attributes with id + link + key
4            + content             + content?

Note that I've renumbered the @verbose setting for single items, the new
default would be 2 (same behaviour as now with @verbose=1).
I don't think having verbose=0 only returning the id for the single-item
case is sane but we could do it for consitency.

This could be orthogonal to a @embed / @attributes setting, the @embed
would override the @verbose. And the @embed could also be specified for
single items in which case we could reduce the shown properties to only
non-multilinks for performance reasons.

Speaking of multilinks, I've thought about a property @multilink which
by default is 1 (show them) and could be set to 0. Especially useful for
fast display of index lists (say in a responsive interface) where
normally multilinks don't make much sense anyway. And of course for all
sorts of searches where multilinks aren't needed.

> >One of the problems I'm seeing is different values of @verbose on
> >/data/issue and /data/issue/1 have different meaning with this
> >implementation. So maybe we never put the label property in there
> >(currently as implemented with @verbose>1), use @verbose=1 for the
> >output example above, make @verbose=1 the default (as in /data/issue/1),
> >provide the old output with @verbose=0 and add links to other items with
> >@verbose=2 and content with @verbose=3 like when requesting a single
> >issue? We could also allow pruning the attributes by specifying an
> >explicit @attributes parameter (also proposed by John some time ago).
> 
> I think @attributes pruning was replaced by @embed in a later discussion.

OK, I didn't notice, why (I'm fine with either variant)

> However there is a more subtle issue with @embed. If I go to
> 
>    curl -u demo:demo 'https://rouilj.dyn amic-dns.net/demo/rest/data/status?@embed=transitions&@verbose=2'
> 
> I get an object like:
> 
>            {
>                 "id": "9",
>                 "link": ".../rest/data/status/9",
>                 "name": "delete",
>                 "transitions": [
>                     "1",
>                     "2",
>                     "4"
>                 ]
>             }
> 
> Note the format of the transitions. It is the same as though
> @verbose=0 was used to access rest/data/status/9 even though I have
> @verbose=2.

See my discussion of verbose above: Verbose=1 would embed all attributes
(unless modified with @embed or @attributes or however we'll call it).
And higher values would specify more expansion

Note that @verbose as currently implemented only works for single items
unless you've implemented something new there.

> There is no way to tell what you can do with a transition from this
> output.  They are statuses and can be retrieved via rest/data/status/1
> etc., but you have to go to rest/data/status/9 with @verbose>0 to see
> that.  If I do a get on rest/data/status/9 with no @verbose, I get the
> transitions expanded:
> 
>         "attributes": {
>             "name": "delete",
>             "requiredpermissions": null,
>             "order": 9,
>             "abbreviation": "d",
>             "transitions": [
>                 {
>                     "id": "1",
>                     "link": ".../rest/data/status/1",
>                     "name": "new"
>                 },
>                 {
>                     "id": "2",
>                     "link": ".../rest/data/status/2",
>                     "name": "open"
>                 },
>                 {
>                     "id": "4",
>                     "link": ".../rest/data/status/4",
>                     "name": "hold"
>                 }
>             ],
>             "help": "Ticket opened by mistake or in error. Considered closed."
>         },
>         "type": "status"
> 
> I was considering refactoring the class and class/id (or key name)
> endpoint code into a single function. So when I asked for:
> 
>    curl -u demo:demo '.../rest/data/status?@embed=transitions&@verbose=2'
> 
> I would get the embedded fields (e.g. transitions) for each element of
> the collection expressed as though I had hit each rest/data/status/1,
> rest/data/status/2, rest/data/status/3...  with @verbose=2.
> 
> So the code re-arrangement would be:
[...]

Yes!

> This would unify the representation of the objects regardless of which
> way they were accessed (class (+ optional embed), class/id (+ optional
> embed)) for a given verbose level.
Yes!

> If the property is requested/shown (either by an explicit @embed, or
> implicitly by requesting the class/id) it always has the exact same
> form based on the @verbose level.
Yes!

But see my discussion above for the verbosity: We'll want to use 0 for
not showing/embedding attributes when listing a class.

> One note, I was discussing this with a developer over the weekend and
> he noted that:
> 
>   @verbose
> 
> might better be named:
> 
>   @detail or @detailevel
>   
> In most Unix commands enabling verbose (-v) doesn't change the command
> output. It produces info about how the output was obtained. In our
> case, it may add a field at the same level as "data" with:
> 
>   "perfdata": { "totalscanneditems": 2100,
>                 "real_time": "2.4s",
> 		"cputime": "1.2s",
> 		"cachemisses": 900
> 	        "cachehits": 3210
>   }
> 
> Not sure I agree with him, but thought I would mention it.
> 
> Thoughts?

I'm not really religious about this naming. I do have a customer testing
(and implementing a client) for this now, so I'm inclined to leave it
as-is... but if you want it changed I'm in favor of the shorter detail
not detaillevel (two 'l'?)

Another thought: We currently can't attach files with binary data via
the REST-API, can we?

Ralf
-- 
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [email protected]