[READY] Drop bottle in favour of a tiny hand-rolled "framework"

[bstaletic]

Turns out ycmd needs a tiny part of the whole framework specification from PEP 3333.
Note that this does not fully conform to the specification, but it mostly does.

Currently we are only handling POST and GET requests.
Support for middleware is limited. Inspired by Bottle plugins API, but only plugin.__call__ and app.install are implemented.
Error handling is also limited. Application needs to replace the default error handler that will handle all errors.

However, this just about fits ycmd's needs.

---

This change is 

[bstaletic]

Reviewable status: 0 of 2 LGTMs obtained

---

ycmd/web_plumbing.py line 73 at r1 (raw file):

class Request:

The request class eagerly processes most things. I thought about some caching, but hmac_plugin.py will definitely need everything anyway.

The only potential improvement is json() and query(), since those are actual function calls.

json() is only called once at the start of every POST callback, so caching won't do anything.

query() is also only called once at the start of every GET callback, so, again, caching won't do anything.

Maybe we could try to avoid setting self.body at all if we are processing a GET request, but I don't think that's a worthwhile effort.

---

ycmd/web_plumbing.py line 114 at r1 (raw file):

    self._routes : Mapping[ str, Mapping[ str, CallbackType ] ] = {
      'GET': {},
      'POST': {} }

Routes are saved as a nested dictionary, so we can access callables as

self._routes[method][path]

For performance, maybe we want to do

self._get_routes[path]
self._post_routes[path]

Need to measure that...

[bstaletic]

Reviewable status: 0 of 2 LGTMs obtained

---

ycmd/web_plumbing.py line 114 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Routes are saved as a nested dictionary, so we can access callables as

self._routes[method][path]

For performance, maybe we want to do

self._get_routes[path]
self._post_routes[path]

Need to measure that...

Measured. Changed to _get_routes and _post_routes.

[codecov]

Codecov Report

Merging #1736 (c00691f) into master (1693a93) will increase coverage by 0.02%.
The diff coverage is 97.19%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1736      +/-   ##
==========================================
+ Coverage   95.46%   95.48%   +0.02%     
==========================================
  Files          83       84       +1     
  Lines        8201     8311     +110     
  Branches      163      163              
==========================================
+ Hits         7829     7936     +107     
- Misses        322      325       +3     
  Partials       50       50              

[puremourning]

This looks really good. Few minor comments. Very pleased with this.

Also, did you try running the YCM client tests too ? Wettest may be a bit of a cheat factor, so I'd like to know that real client tests are working too. (there's also client_test.py which is a good litmus, and that clearly passes).

Reviewed 8 of 9 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

---

ycmd/__main__.py line 50 at r2 (raw file):

    os.path.join( DIR_OF_REQUESTS_DEPS, 'urllib3', 'src' ),
    os.path.join( DIR_OF_WATCHDOG_DEPS, 'watchdog', 'build', 'lib3' ),
    os.path.join( DIR_OF_WATCHDOG_DEPS, 'pathtools' ),

is pathtools not still needed by watchdog? (or is this an existing error) ?

---

ycmd/web_plumbing.py line 60 at r2 (raw file):

  def __getattribute__( self, query_name : str ) -> str:
    return super().get( query_name, [ '' ] )[ 0 ]

it's not immediately obvious why this is expecting a list response, nor why the 0th element is the right one.

Is it essentially because you can have the same query string parameter multiple times and parse_qa just always returns a list [ value ] in the case of only one?

---

ycmd/web_plumbing.py line 96 at r2 (raw file):

def _ApplyPlugins( plugins, callback ):

It took me way too long to understand what was going on here. Not being familiar with bottle's plugin system, I was like wtf is callback = plugin( callback ) doing...?

But it's basically creating a set of decorators with decorate the underlying handler (original callback value) with wrappers (return values of plugin(callback)).

Maybe stick that in a comment, because I will forget....!

---

ycmd/web_plumbing.py line 196 at r2 (raw file):

      response = Response()
      out = self._ErrorHandler( e, response )
    except Exception:

I think if someone sends an invalid route the returned error will be 500 KeyError or something.
I think that's probably OK, though I was half-expecting 404.

I checked the API docs and 404 is never mentioned, so I think it's fine, While the error message will be confusing to users, it represents a programming error that only ycmd/client devs will ever see.

---

ycmd/web_plumbing.py line 205 at r2 (raw file):

      out = self._ErrorHandler( e, response )
    out = out.encode( 'utf-8' )
    response_headers = response.headers

this assignment looks fishy. It looks like you're trying to copy response.headers but actually you're taking a pointer to it, then mutating it on the next line.

I have no issue with mutating it (it's probably correct), but just want to check that's what you were intending.

[bstaletic]

I have no ran YCM tests. I'll set up my YCM fork, dispatch CI and report the results.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

ycmd/__main__.py line 50 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

is pathtools not still needed by watchdog? (or is this an existing error) ?

It's been a long time since my pull request that got watchdog off pathtools and onto pathlib, which is the standard one.
We have just never removed that thing from the path.

---

ycmd/web_plumbing.py line 60 at r2 (raw file):

Is it essentially because you can have the same query string parameter multiple times and parse_qa just always returns a list [ value ] in the case of only one?

Yes.

>>> urllib.parse.parse_qs('a=b&a=3')
{'a': ['b', '3']}
>>> urllib.parse.parse_qs('a=b&c=d')
{'a': ['b'], 'c': ['d']}

Maybe we rather want index -1?
Should I put that in a comment?

---

ycmd/web_plumbing.py line 96 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

It took me way too long to understand what was going on here. Not being familiar with bottle's plugin system, I was like wtf is callback = plugin( callback ) doing...?

But it's basically creating a set of decorators with decorate the underlying handler (original callback value) with wrappers (return values of plugin(callback)).

Maybe stick that in a comment, because I will forget....!

Fair. Decorator stuff easily gets into the head-scratcher territory.
Done.

---

ycmd/web_plumbing.py line 196 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I think if someone sends an invalid route the returned error will be 500 KeyError or something.
I think that's probably OK, though I was half-expecting 404.

I checked the API docs and 404 is never mentioned, so I think it's fine, While the error message will be confusing to users, it represents a programming error that only ycmd/client devs will ever see.

Yeah, I was lazy there. It definitely will return 500 KeyError.

It's a bit awkward to differentiate KeyError because the path does not exist and KeyError because the callback has thrown that. Latter of which we do actually want as 500.

Solving that requires something like

try:
  callback = routes[path]
except KeyError:
  # return 404
else:
  callback( request, response )

Which is not bad, but two thoughts crossed my mind:

1. When wasa the last time you have seen a try/else?
2. We should have a test for this 404 response.

---

ycmd/web_plumbing.py line 205 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

this assignment looks fishy. It looks like you're trying to copy response.headers but actually you're taking a pointer to it, then mutating it on the next line.

I have no issue with mutating it (it's probably correct), but just want to check that's what you were intending.

Yeah, that line is silly. I'd prefer

response.add_header(...)
start_response( status, response.headers )

Mutating is fine, but as written it does look fishy.

Fixed.

[bstaletic]

Good news, everyone!
https://github.com/bstaletic/YouCompleteMe/actions/runs/7978280435

Vim survived this branch.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

[bstaletic]

Let's see if tests will pass. If they do, and the vim tests pass as well (again), I might merge this.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

---

ycmd/web_plumbing.py line 60 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Is it essentially because you can have the same query string parameter multiple times and parse_qa just always returns a list [ value ] in the case of only one?

Yes.

>>> urllib.parse.parse_qs('a=b&a=3')
{'a': ['b', '3']}
>>> urllib.parse.parse_qs('a=b&c=d')
{'a': ['b'], 'c': ['d']}

Maybe we rather want index -1?
Should I put that in a comment?

I have tried a few websites to see what they do regarding repeated query parameters. One took the first value, one took the last value. Leaving as is.

---

ycmd/web_plumbing.py line 196 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Yeah, I was lazy there. It definitely will return 500 KeyError.

It's a bit awkward to differentiate KeyError because the path does not exist and KeyError because the callback has thrown that. Latter of which we do actually want as 500.

Solving that requires something like

try:
  callback = routes[path]
except KeyError:
  # return 404
else:
  callback( request, response )

Which is not bad, but two thoughts crossed my mind:

1. When wasa the last time you have seen a try/else?
2. We should have a test for this 404 response.

Implemented, with a test. Now the message in vim is

RouteNotFound: '/the_route'

[bstaletic]

Reviewed 8 of 9 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

[bstaletic]

All review comments have been addressed. Ycmd and YCM tests are passing and a manual test with unicode identifiers passed as well. Merging.