Copy link Copy Markdown

Contributor

Copy link Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature added so that Arc<ServerLimits> can be serialized to a JSON response payload. The alternative is to derive Clone and Copy on ServerLimits and don't keep it in an Arc. I wasn't sure which would be preferable, happy to make the switch if this way is wrong.

Copy link Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to get away w/out it. The inner ServerLimits can be deref'd out of the arc via *server.limits -- and we need to borrow it for the json call, so: json(&*server.limits)

Feel free to change this later if it makes it easier on these 2 outstanding PRs

Copy link Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did wonder whether this test is too tautological to be useful, but I think it's okay. Asserting that the response payload equals the JSON serialization of the default ServerLimits is fine, yeah?

Copy link Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably retain the doc strings as well.

Copy link Copy Markdown

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only learned this for the first time when I reviewed one of those pull requests yesterday. Hence going back and applying it to the other code I wrote in here, even though it's not strictly related.

Copy link Copy Markdown

Contributor Author

It just occurred to me that I haven't done anything in this PR to actually enforce these limits, I'm just blithely using the same values as the Python codebase. Is it something that we'd configure in nginx or whatever, or are there hooks in actix-web we can use? (I'll go and RTFM right now, sorry)

Anyway, hopefully it's okay for a follow-up (I'll open an issue if so)...

EDIT: #40

Copy link Copy Markdown

Contributor Author

Is it something that we'd configure in nginx or whatever, or are there hooks in actix-web we can use?

(I mean the _BYTES settings here of course, the _RECORDS ones will need to be enforced in code I realise)

EDIT: Actually, thinking it through sorry, it's only max_request_bytes that could be enforced outside of code anyway. Ignore me.

Closed

pjenvey previously approved these changes Sep 28, 2018

Copy link Copy Markdown

Contributor Author

Sorry both, I had to rebase because merge conflicts, so your reviews were dismissed.

On the plus side it was an opportunity to try Phil's suggestion about dereffing state.limits, which worked well. So, winning really.