resurfaceio / logger-python

Log API calls with Python
Apache License 2.0
22 stars 6 forks source link

Fix/middleware issues #55

Closed Anyesh closed 3 years ago

Anyesh commented 3 years ago
monrax commented 3 years ago

It looks great! I think I have mainly three questions:

Anyesh commented 3 years ago

It looks great! I think I have mainly three questions:

  • Does the multipart header always end with --[something here]? (if it doesn't have those two hyphens together, the regex won't match)

The answer would be yes! I did check that with some custom test apps besides the HTTP tools and this is the body that I am getting.

# From the web app
------WebKitFormBoundaryTaEhyl7YlQcb57S4 Content-Disposition: form-data; name="title" Sombraa ------WebKitFormBoundaryTaEhyl7YlQcb57S4 Content-Disposition: form-data; name="image"; filename="780127.jpg" Content-Type: image/jpeg <file-data> ------WebKitFormBoundaryTaEhyl7YlQcb57S4--

# From the HTTP tool
--X-INSOMNIA-BOUNDARY Content-Disposition: form-data; name="title" apple --X-INSOMNIA-BOUNDARY Content-Disposition: form-data; name="image"; filename="0c3a0a0f7678e5cc16463bc36f498242.jpg" Content-Type: image/jpeg <file-data> --X-INSOMNIA-BOUNDARY--
  • Django middleware doesn't necessarily need to be at the top of the stack. It just needs to be the first to access the request.body property, before any other middleware (or view) calls request.read() instead. Putting it at the top certainly guarantees this. However, it also works fine when placing it below all the middleware that comes with a normal django project. Should we say something like "place it before any custom middleware" instead?

Sure! Just to be safe I mentioned putting that on the top but we can rephrase that.

  • I modified http_logger.py; I moved the warnings inside the ResurfaceWarning class so that they are all in the same place. We should create a warning in case USAGE_LOGGERS_DISABLED env var has something different from a boolean. That one is read at base_logger.py. Should we move ResurfaceWarning to utils so it can be used in other places?

Sure! I was trying to arrange all that on the utils folder but I am planning to do all that from the restructure branch so for now I have left utils in the separate python files. And since that warning class was used only inside the http_logger I left that on there but feel free to pull that out in a separate python file and we can arrange that later inside a folder.

monrax commented 3 years ago

Cool, thanks! I just moved the new ResurfaceWarning class to its own file, to be grouped later with the other utils. I also wrapped the response body assignment inside a try-except block, in case the response doesn't have a content attribute.