search

WIPACrepo / file_catalog

Store file metadata information in a file catalog
MIT License
1 stars 4 forks source link

Server Spring Cleaning: Code Maintenance & Bug Fixes #104

Closed ric-evans closed 3 years ago

ric-evans commented 3 years ago
ric-evans commented 3 years ago

now only 5 tests failing, making progress :sweat_smile:

ric-evans commented 3 years ago

I need to add tests for the use cases laid out in the linked issue. Other than that, it's ready for review.

Also fixed a potential bug where self.assertRaises(Exception) was being used. The exception that was caught was actually being raised by the testing code (referencing an undeclared variable), but there was no way to tell the difference. So I added a check. That's my PSA.

ric-evans commented 3 years ago

@blinkdog can you look at https://github.com/WIPACrepo/file_catalog/blob/patch-put-bug-fixes/file_catalog/server.py#L727 ish? I don't think your note here is accurate:

note that if we get the record that we are trying to update the location will NOT be added to the list of new_locations which leaves new_locations as a vetted list of addable locations

lgtm-com[bot] commented 3 years ago

This pull request fixes 16 alerts when merging 388262e1b70226574b68f5075460633b4e3ccc0c into 34443618326517757d185b2b7c7a4f1934cf0115 - view on LGTM.com

fixed alerts:

ric-evans commented 3 years ago

@dsschult @blinkdog Okay, I've finished everything I planned to do, plus all the incidental things (incidental >> planned). It's ready for review.

ric-evans commented 3 years ago

@dsschult for many of the send_error() calls, we're including file and/or location. Looking at APIHandler.write_error() (https://github.com/WIPACrepo/file_catalog/blob/patch-put-bug-fixes/file_catalog/server.py#L396), these args are written to the output buffer. But I don't see how that's visible to the client. And in my tests, I haven't seen these show up in the request's http errors.

This is also why I refactored message to reason. reason is visible to the client. So no more bland "Bad Request". Not sure if this was the intended behavior and never got addressed... but it sure was a rabbit hole :smile:

dsschult commented 3 years ago

@dsschult for many of the send_error() calls, we're including file and/or location. Looking at APIHandler.write_error() (https://github.com/WIPACrepo/file_catalog/blob/patch-put-bug-fixes/file_catalog/server.py#L396), these args are written to the output buffer. But I don't see how that's visible to the client. And in my tests, I haven't seen these show up in the request's http errors.

This is also why I refactored message to reason. reason is visible to the client. So no more bland "Bad Request". Not sure if this was the intended behavior and never got addressed... but it sure was a rabbit hole smile

I think this was intended to log the message, but not output it back to the client. I know it's bad practice to output unsanitized input back to the client,

ric-evans commented 3 years ago

@dsschult for many of the send_error() calls, we're including file and/or location. Looking at APIHandler.write_error() (https://github.com/WIPACrepo/file_catalog/blob/patch-put-bug-fixes/file_catalog/server.py#L396), these args are written to the output buffer. But I don't see how that's visible to the client. And in my tests, I haven't seen these show up in the request's http errors. This is also why I refactored message to reason. reason is visible to the client. So no more bland "Bad Request". Not sure if this was the intended behavior and never got addressed... but it sure was a rabbit hole smile

I think this was intended to log the message, but not output it back to the client. I know it's bad practice to output unsanitized input back to the client,

Alright, that's what I was thinking. I haven't removed any of those "extra" arguments. So, it's still backwards compatible, aside from message/reason.

ric-evans commented 3 years ago

@blinkdog One thing I'd like your input about is the several mypy errors (mypy: error override - Signature of "initialize" incompatible with supertype "APIHandler") for the derived class' initialize() methods. Since we're using kwargs and immediately calling super().initialize(**kwargs) it's not terrible. But mypy disagrees.

lgtm-com[bot] commented 3 years ago

This pull request fixes 16 alerts when merging 6362164f0e147cbb9ae6a00c31d9b1998b2cc1eb into 34443618326517757d185b2b7c7a4f1934cf0115 - view on LGTM.com

fixed alerts:

blinkdog commented 3 years ago

@blinkdog One thing I'd like your input about is the several mypy errors (mypy: error override - Signature of "initialize" incompatible with supertype "APIHandler") for the derived class' initialize() methods. Since we're using kwargs and immediately calling super().initialize(**kwargs) it's not terrible. But mypy disagrees.

So the superclass of APIHandler (one of ours) is RequestHandler (from Tornado) and the docs give it this signature: https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.initialize

We can use kwargs, but I think replicating the signature down is probably the right way to go. The tests confirm that it's not a correctness issue, but I think taking mypy's advice is a good idea.

lgtm-com[bot] commented 3 years ago

This pull request fixes 16 alerts when merging 654a813f29a2530e05e6c8333699b6683f2cf12f into 34443618326517757d185b2b7c7a4f1934cf0115 - view on LGTM.com

fixed alerts:

dsschult commented 3 years ago

@blinkdog One thing I'd like your input about is the several mypy errors (mypy: error override - Signature of "initialize" incompatible with supertype "APIHandler") for the derived class' initialize() methods. Since we're using kwargs and immediately calling super().initialize(**kwargs) it's not terrible. But mypy disagrees.

So the superclass of APIHandler (one of ours) is RequestHandler (from Tornado) and the docs give it this signature: https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.initialize

We can use kwargs, but I think replicating the signature down is probably the right way to go. The tests confirm that it's not a correctness issue, but I think taking mypy's advice is a good idea.

I'll disagree with @blinkdog about removing kwargs. Both *args and **kwargs are fundamental parts of Python, in that you can "pass along" arguments without explicitly knowing what they are. This is very handy when creating a wrapper around something, since the underlying object can change its arguments and your library doesn't need to be updated (even if your users have to update their code).

I also believe this is a step too far in pursuing fully typed Python - we might as well switch to a fully typed language like Rust then :smile:, as I think we start to lose some of the strengths of Python.

ric-evans commented 3 years ago

@blinkdog One thing I'd like your input about is the several mypy errors (mypy: error override - Signature of "initialize" incompatible with supertype "APIHandler") for the derived class' initialize() methods. Since we're using kwargs and immediately calling super().initialize(**kwargs) it's not terrible. But mypy disagrees.

So the superclass of APIHandler (one of ours) is RequestHandler (from Tornado) and the docs give it this signature: tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.initialize We can use kwargs, but I think replicating the signature down is probably the right way to go. The tests confirm that it's not a correctness issue, but I think taking mypy's advice is a good idea.

I'll disagree with @blinkdog about removing kwargs. Both *args and **kwargs are fundamental parts of Python, in that you can "pass along" arguments without explicitly knowing what they are. This is very handy when creating a wrapper around something, since the underlying object can change its arguments and your library doesn't need to be updated (even if your users have to update their code).

I also believe this is a step too far in pursuing fully typed Python - we might as well switch to a fully typed language like Rust then , as I think we start to lose some of the strengths of Python.

I do find it surprising that mypy can't track a parameter via kwargs to its parent class.

ric-evans commented 3 years ago

I will add, in support of type-hinting, running mypy will only complain if there are errors. So any legacy/non-typed code is essentially ignored.

Locally, I run with the --strict flag, but that's overkill for our CI needs.

lgtm-com[bot] commented 3 years ago

This pull request fixes 16 alerts when merging d8b792bb7f5ba72f1019ec1e36767011d795661e into 34443618326517757d185b2b7c7a4f1934cf0115 - view on LGTM.com

fixed alerts:

lgtm-com[bot] commented 3 years ago

This pull request fixes 16 alerts when merging 7a484d2fd40f871ced1e56b01e655c632998d3a2 into 4483be7e3f32c74b91f1eef98090b729c436be02 - view on LGTM.com

fixed alerts: