[21:02:22] #startmeeting TechCom RFC discussion [21:02:22] Meeting started Wed May 29 21:02:22 2019 UTC and is due to finish in 60 minutes. The chair is duesen. Information about MeetBot at http://wiki.debian.org/MeetBot. [21:02:22] Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. [21:02:22] The meeting name has been set to 'techcom_rfc_discussion' [21:02:30] #topic [21:02:44] #topic Route Handler Interface RFC [21:02:52] #link https://phabricator.wikimedia.org/T221177 [21:03:31] who is here to discuss the REST API? [21:03:54] apparently not ebernhar|lunch ;) [21:03:59] tgr, anomie? [21:04:20] o/ [21:04:39] maybe evanpro wants to join us as well [21:04:45] I'll lurk a bit before I fall asleep [21:05:18] I'm not sure it'll make you have nice dreams :) [21:05:34] TimStarling: do you want to give a brief intro, and ask key questions to discuss? [21:05:44] Having a REST API in MediaWiki has been discussed on and off for a couple of years now. But this quarter, Core Platform Team is fully engaged in building it. The immediate use case is the port of Parsoid to PHP, which is also underway. [21:05:52] We want to provide an interface to Parsoid which is similar enough to Parsoid's existing REST API that RESTBase can use either the JS or PHP version of Parsoid. [21:06:01] The current RFC is a discussion of the Handler and Router of the new API, and a few supporting classes. The aim of this meeting is: [21:06:10] * To introduce the REST API for anyone who isn't aware of the project [21:06:19] * To get approval for the extension interface, so that we can stabilise it and use it in extensions without fear of immediate deprecation. [21:06:41] there are a lot of questions, some of them are asked in the task description [21:07:53] My main question is "when will it land?". :-) [21:08:00] #link https://gerrit.wikimedia.org/r/c/mediawiki/core/+/508972/12 [21:08:22] land in core or WMF production? [21:08:25] there has been a fair amount of discussion om the task, are there specific questions you'd like to discuss here TimStarling? [21:08:35] maybe thedj wants to join as well, he was active on the task [21:08:38] TimStarling: Both. [21:08:56] Oh btw, before we get carried away in the discussion: this is to remind everyone to make liberal use of the #info marker, to populate the minutes [21:09:41] anomie was promising yesterday to have some sort of parameter validation thing done in time for our offsite in June 17-21 [21:10:01] probably the rest of it will be more or less together by then [21:10:20] so I imagine it will be in core by around that time, although possibly hidden by a feature flag [21:10:52] as for deployment, well, we probably need a use case before we deploy it [21:11:07] Other than Parsoid? [21:11:22] by extension interface we mean extension.json, not the handler interface, right? I imagine the latter will change a fair bit, with parameter validation, documentation, caching etc [21:11:34] I don't know if that will mean Parsoid or some basic page content interface [21:12:20] tgr: ideally I would like the handler interface to only change in a backwards-compatible way going forward [21:12:29] that would mean documentation etc. would be optional [21:12:48] we can add methods to the Handler class, as long as they are not abstract [21:12:59] As far as I known, evanpro wants us to build a CRUD interface for page edits as the first (non-parsoid related) interface [21:13:26] R is simple enough, I don't know about CUD [21:13:55] heh. i have some ideas ;) [21:14:19] but we are drifting towards discussing specific endpoints, which is out of scope for this rfc [21:14:41] no one in the WMF seems to be rushing to use the API, other than the parsing team, and most everyone else won't be affected until the next release, so there is little value in freezing the handler API now, IMO [21:14:53] not that I have any specific problem with it, just noting [21:15:25] to get things started, let me ask a question from the task description: [21:15:27] "The current path template matcher allows only string literals and wildcards. Conflicting paths are not allowed. Is this sufficient? This means that for example the existing RESTBase routes /lists/setup and /lists/{id} would conflict, preventing registration" [21:15:41] I'm not really saying I want to freeze it, I'm saying people should ideally voice their objections now because it's easier to fix them now than in a month [21:16:42] duesen: that's probably the only conflict, services had to change path handling in RESTBase to make those two routes happen, and that was not so long ago [21:18:00] also, introducing typed wildcards would be a backwards-compatible change, if it turns out to be useful [21:19:08] I think there was also a comment from anomie to the effect that typed wildcards could be done and maybe that they would be a good idea [21:19:30] most REST frameworks just have a bunch of path regexes that they run in some priority order [21:19:42] that seemed slow to me, given that we will probably have many many routes [21:19:57] For the CRUD endpoint, I could imagine a route that serves GET for revision/12345/main and one that handles POSTs to revision/new/main. [21:20:16] that's why I implemented a routing tree, where you just have a hashtable to consult for each path component [21:20:37] actually... could it be useful to have different handlers for different methods on the same path? [21:20:49] I'd expect if you squash everything into a single huge regexp you get decent performance, no conflict detection though [21:21:02] duesen: yes, we have that already [21:21:14] ok good [21:21:29] tgr complained that it doesn't emit 405 errors when another method would be matched [21:22:06] right [21:22:36] well, for that we would need a combined routing tree, just for 405s [21:23:11] I'm noting that we are not actually discussing the extension point. The RFC is called "REST route handler extension interface RFC". [21:23:21] it's probably possible to do that, especially if it doesn't need to be perfect [21:23:26] 405s are not worth putting a lot of effort into [21:23:26] so, what are the relevant extension points? [21:23:31] what would extensions have to do? [21:24:10] extensions register routes in their extension.json [21:25:08] the route object in extension.json includes a way to construct a Handler object for that route [21:25:56] We were at some point talking about decoding payloads. [21:26:09] IIRC, Handlers have to do their own decoding and validation. [21:26:12] the extension interface includes the Handler base class, RequestInterface and ResponseInterface [21:26:15] Will tehre be any help with that? [21:26:46] there will be help with validation, anomie is working on that [21:27:04] I imagine there will be a method like Handler::getValidator() [21:27:20] rather than having the whole validation framework in the Handler base class [21:27:25] as is done in the API [21:27:30] *action API [21:27:34] AFAIK anomie is working on parameter validation, not validation of POST bodies. [21:28:02] Yes, composition seems like the correct approach [21:28:18] there's no validation of POST bodies proposed for core [21:28:44] I think tgr was talking about maybe having JSON schema validation? [21:29:21] yeah, I think anomie intends to provide an interface for that and I'll look into the implementation [21:29:47] we removed RequestInterface::getParsedBody() [21:29:47] which is basically just picking which library to use [21:30:23] I sort of disagree with the removal, but that would not have included validation anyway [21:30:33] apart from ensuring it is valid JSON [21:30:52] I still think a wrapper that has methods like getInt( $path ) would go a long way as some kind of ad-hoc validation. [21:31:57] speaking of RequestInterface, there's a comment from tgr that RequestInterface::getAttributes() should not just return the path parameters [21:31:57] * duesen will read back and hunt for some quites to go into #info lines [21:32:17] it should return something like [ 'pathParams' => ... ] [21:32:30] well, attributes were originally mainly meant for extension points [21:32:36] middleware, especially [21:32:41] #info there's no validation of POST bodies proposed for core [21:32:52] #info TimStarling> the extension interface includes the Handler base class, RequestInterface and ResponseInterface [21:33:00] we talked a bit about middleware early on but I don't think we arrived to any decision [21:33:37] there's no middleware concept in core [21:33:47] although of course extensions can implement their own middleware concept [21:33:59] of course having the ability to use middleware becomes a lot less useful if we can't provide PSR-7 compatibility [21:34:37] you are saying there is PSR-7 middleware you would want to use? [21:34:58] PSR-7 compatibility actually sounds like a pretty good topic. [21:35:01] I expect there would be [21:35:21] like what? [21:35:35] IIRC, Tim decided against "true" PSR-7 compatibility, but we are still trying to be "similar" to PSR-7. Is that correct, and why are we doing it this way? [21:35:58] authentication / crypto stuff, for example [21:36:16] JWT decoding was the example I mentioned on the task [21:37:00] we are doing things similarly to PSR-7 because it was a nice conceptual starting point, they had some good ideas [21:37:11] we are not using PSR-7 because they also had some bad ideas [21:37:27] especially immutable responses [21:37:37] the main patch is PSR-7 compatible [21:37:53] in the sense that it's easy to convert to/from PSR-7 objects if needed [21:38:17] #info we are doing things similarly to PSR-7 because it was a nice conceptual starting point, they had some good ideas [21:38:20] JsonResponse is not, it's one of the reasons I'm unsure if it's a good idea [21:38:33] #info TimStarling> we are not using PSR-7 because they also had some bad ideas, especially immutable responses [21:40:40] There is another question in the task description that I find interesting: [21:40:41] "Do iteration/index routes require any special handling?" [21:41:30] Paging is a recurring theme, and something that we are doing in multiple places in core, and have gotten wrong a few times. [21:41:48] special handling in what sense? I doubt they require a different interface [21:42:02] some kind of core helper to ensure uniformity, sure [21:42:06] Is there a way to have some kind of generic paging support? Or are the needs of different use cases too specific? [21:42:27] tgr: what would that helper look like? [21:43:22] define a standard index parameter, output a standard position parameter [21:43:34] basically just enforce naming conventions [21:43:59] a query parameter? [21:44:00] maybe make that parameter tamper-proof, there is a debate on the value of that in the task [21:44:04] yes [21:44:51] I think that's the most common approach for REST APIs [21:45:17] Intuitively, a query parameter seems the right thing for paging. [21:45:54] at least for listings that can't really be cached [21:46:10] in theory you could use Range headers, but it makes debugging less convenient and there is no benefit other than being able to claim restfulness cred [21:46:37] bein able to reference a specific "page" with a url is useful [21:47:33] #info for paging, use a (standard) query parameter, and a (standard) field on the output. headers could also be used, but are more obswcure [21:49:14] what are the next steps, given that this meeting has attracted no new perspectives and all three of the people talking have written 50+ comments on the subject already [21:50:07] perhaps post on wikitech-l? [21:50:20] But in general, I have found the best way to attract attention is to break stuff ;) [21:50:22] move to last call? a two week last call period roughly corresponds with the timeline for having everything in core [21:51:15] i think we can go to last call if you think the open questions are suffiociently answered. [21:51:45] a lot of them don't really need to be answered yet [21:51:49] if the API is not meant to be frozen, I'm not sure what last call would even mean [21:52:34] although issuing last calls is always a good way to get more attention on an RfC :) [21:52:50] a last call is the only way to resolve an RFC, IIRC [21:53:38] I can move the open questions from the task description to a comment [21:55:10] We didn't talk about putting this on last call during the meeting today. Perhaps send an email to techcom members first, so it doesn't come as a surprise. or wait until the next meeting to start the last call period [21:55:28] there seems to be open questions about localisation, but it seems that what is proposed doesn't really do anything about it nor rule it out being added later? [21:55:54] i think the questions can stay in the description, if their status and relevance is clear. [21:57:28] Nikerabbit: right... the error handling part could be fleshed out a bit more. [21:57:50] ResponseFactory may need some i18n [21:58:06] have a look at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/512775/ [21:58:15] the question is how much we are worried about cache splitting [21:58:27] if we aren't, we should add i18n [21:58:59] I'm dubious about that to be honest... it seems like putting presentation logic into the wrong place. but then, human readable error messages are rather convenient. and let's please not use numeric error codes. for anything, ever... [21:59:27] tgr: for errors only, I assume? [21:59:32] well, the other question is whether we need to differentiate between content language and response language, and how that would work [22:00:00] #info TimStarling> ResponseFactory may need some i18n [22:00:00] for endpoints that need to present responses to users, they will care about i18n more than cache splitting [22:00:14] duesen: for errors, yeah. Where the content needs i18n, that will happen outside the API framework anyway [22:00:20] I mean, the question is whether you have two requests, one to do the thing and another to fetch messages, or if you bundle [22:01:02] #info [de] we need to differentiate between content language and response language [22:01:37] TimStarling: yes, I agree. and errors usually won't be cacheable anyway [22:01:43] the current WMF mobile apps prefer to fetch error keys and do their own i18n, AFAIK [22:01:45] ok, we are out of time. [22:01:54] as always ,that'S when the interesting discussion starts [22:02:20] tgr: clients should have that option, yea. but should they be forced to?... [22:02:22] anyway. [22:02:25] I'll close the meeting now [22:02:34] it seems handy to be able to request localised error messages, but then again the client knows better and can give better error message... one thing that the action api lacks is a list of possible errors that can happen for a given request [22:02:39] we should have an RfC on RfCs some time [22:02:55] we can continue the conversation on the ticket, or in #wikimedia-tech [22:03:05] #endmeeting [22:03:06] Meeting ended Wed May 29 22:03:05 2019 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) [22:03:06] Minutes: https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-05-29-21.02.html [22:03:06] Minutes (text): https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-05-29-21.02.txt [22:03:06] Minutes (wiki): https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-05-29-21.02.wiki [22:03:06] Log: https://tools.wmflabs.org/meetbot/wikimedia-office/2019/wikimedia-office.2019-05-29-21.02.log.html [22:03:17] yeah need zZZ [22:03:22] tgr: you mean like https://phabricator.wikimedia.org/T216308 ? [22:03:27] I feel for something so basic and wide-ranging as changing the API paradigm, if no one shows up to the discussion, we must be doing something wrong [22:03:54] I can understand if very few people feel able to comment on, say, MCR [22:04:11] tgr: well, the scope of this rfc is rather narrow. another rfc will be needed for the CRUD endpoint. that's much more visible to the outside [22:04:19] but a lot of people have worked with other APIs, and other API frameworks [22:06:07] I used to care more about getting people into these meetings [22:06:20] we could have an RFC proposing to retire the action. I'm sure that wopuld draw attention :) [22:07:16] I would write a separate post to wikitech-l encouraging poeple to join, then when the meeting started, I used to write a reminder on a few IRC channels [22:07:41] TimStarling: I care more about getting the right people top comment on phab. do you think the discussion there is getting anough attention? [22:08:10] hard to say [22:08:40] it probably needs some more comments from the kinds of people who write clients [22:09:01] I think this one is a bit abstract as an RfC, like duesen said it doesn't really affect much people as there aren't concrete plans to make it the default/mandatory/recommened API [22:10:01] TimStarling: but these are nto affected by the *extension* interface of the rest router... [22:10:31] they care about the endpoints we are going to offer [22:11:50] yeah, but on response formatting, paging, request parameters and JSON request bodies they might have some opinions [22:12:25] got to go [22:15:56] duesen: You missed "15:11:51 yeah, but on response formatting, paging, request parameters and JSON request bodies they might have some opinions"