Added convenience variables for middleware and exception handlers

[ml-evs]

Closes #536.

[codecov]

Codecov Report

Merging #537 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #537      +/-   ##
==========================================
- Coverage   91.51%   91.47%   -0.04%     
==========================================
  Files          62       62              
  Lines        3123     3110      -13     
==========================================
- Hits         2858     2845      -13     
  Misses        265      265              

Flag	Coverage Δ
#project	91.47% <100.00%> (-0.04%)	⬇️
#validator	63.53% <100.00%> (-0.16%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
optimade/server/exception_handlers.py	83.33% <100.00%> (+0.52%)	⬆️
optimade/server/main.py	94.23% <100.00%> (-0.86%)	⬇️
optimade/server/main_index.py	94.44% <100.00%> (-0.72%)	⬇️
optimade/server/middleware.py	95.00% <100.00%> (+0.06%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3917e44...7310237. Read the comment docs.

[CasperWA]

Seems good.

As you point out yourself, we might want to do this for the the exceptions handlers as well, however, this does not seem as straight forward since you need to match the handlers up with Exceptions. But if you have a good idea in mind, I don't mind merging this PR with two distinct commits for each of these (middleware + exception handlers).

Also, do you think REQUIRED_MIDDLEWARE is the best name? Specifically thinking about the REQUIRED part. Maybe just OPTIMADE_MIDDLEWARE?

I have accepted this PR though, since I have no strong opposing opinion on the above points, and am fine with it as is. However, a comment on each point would be good before merging.

[ml-evs]

As you point out yourself, we might want to do this for the the exceptions handlers as well, however, this does not seem as straight forward since you need to match the handlers up with Exceptions. But if you have a good idea in mind, I don't mind merging this PR with two distinct commits for each of these (middleware + exception handlers).

Done, didn't seem so bad. Side effect is that the index server will also handle NotImplementedError.

Also, do you think REQUIRED_MIDDLEWARE is the best name? Specifically thinking about the REQUIRED part. Maybe just OPTIMADE_MIDDLEWARE?

I agree that's clearer, so updated it.

I have accepted this PR though, since I have no strong opposing opinion on the above points, and am fine with it as is. However, a comment on each point would be good before merging.

I also added docstrings and typehints to the new constants, which mkdocstrings handles quite nicely (note the docs for the exception_handlers module are currently non-existent, though I'm not over-worried about this...)

[CasperWA]

Nice with the doc strings!

The CORS middleware should be added before our custom middleware due to AddWarnings needing to be last.
It's a bit worrying the tests are not catching this, maybe it was too difficult setting one up properly? But this is what I remember from creating the middleware, and what I also put into its doc string.

[ml-evs]

But this is what I remember from creating the middleware, and what I also put into its doc string.

Where does it say this in the docs? Struggling to find it. Might be worth adding a note in the docstring of OPTIMADE_MIDDLEWARE too.

[CasperWA]

But this is what I remember from creating the middleware, and what I also put into its doc string.

Where does it say this in the docs? Struggling to find it. Might be worth adding a note in the docstring of OPTIMADE_MIDDLEWARE too.

Yeah, just read it through. What I remembered was this, but that obviously has nothing to do with when to add it to the application.
I should add this. Maybe see if I have written something about it in a commit or PR where this was added.

[ml-evs]

I should add this. Maybe see if I have written something about it in a commit or PR where this was added.

Cool, feel free to add it to this PR where/when you please (there's no rush to get this in)

[CasperWA]

I should add this. Maybe see if I have written something about it in a commit or PR where this was added.

Cool, feel free to add it to this PR where/when you please (there's no rush to get this in)

Done.
I have added some text to the AddWarnings middleware doc string. Feel free to distribute it around if you feel it's necessary, but maybe we should just link to the middleware and exception handlers instead in the convenience variables?

[CasperWA]

Fine for me now.
As stated above, if you wish to distribute the text I added in other places, feel free, I will re-review later then :)

[CasperWA]

Looks great now. I've again approved, since my suggested change is not crucial, I just thought it might make the note look more accessible by having a "one line summary".

[ml-evs]

I'll just quickly rebase this into 2 commits; one for middleware by both of us and one for exception handlers by me

[ml-evs]

I'll just quickly rebase this into 2 commits; one for middleware by both of us and one for exception handlers by me

Done, so much for a short and sweet PR!

[CasperWA]

Cheers @ml-evs !