Skip to content

Conversation

ekalosak
Copy link
Contributor

@ekalosak ekalosak commented Sep 11, 2023

Summary

I removed an invalid argument to a logging.warning call.

Related

#130

What was broken

As-is, the logging block for missing content-type headers is:

def _on_call_valid_content_type(request: _Request) -> bool:
    """Validate content"""
    content_type: str | None = request.headers.get("Content-Type")

    if content_type is None:
        _logging.warning("Request is missing Content-Type.", content_type)
        return False
...

In the conditional block, you can see the None positional arg content_type passed to the logging call. Python logging doesn't allow null positional (nor keyword) args. The logging used in this module is indeed stdlib Python logging cf functions-framework-python from which it is imported.

A minimal reproducible example

Using Python 3.11.4, python -c 'import logging; logging.warning("this will raise an exception", None)'

Expected result

--- Logging error ---
Traceback (most recent call last):
  File "/.../3.11.4/lib/python3.11/logging/__init__.py", line 1110, in emit
    msg = self.format(record)
          ^^^^^^^^^^^^^^^^^^^
  File "/.../3.11.4/lib/python3.11/logging/__init__.py", line 953, in format
    return fmt.format(record)
           ^^^^^^^^^^^^^^^^^^
  File "/...versions/3.11.4/lib/python3.11/logging/__init__.py", line 687, in format
    record.message = record.getMessage()
                     ^^^^^^^^^^^^^^^^^^^
  File "/.../python3.11/logging/__init__.py", line 377, in getMessage
    msg = msg % self.args
          ~~~~^~~~~~~~~~~
TypeError: not all arguments converted during string formatting
Call stack:
  File "<string>", line 1, in <module>
Message: 'this will raise an exception'
Arguments: (None,)

@ekalosak ekalosak closed this Sep 11, 2023
@ekalosak ekalosak reopened this Sep 11, 2023
Copy link
Contributor

@exaby73 exaby73 left a comment

Choose a reason for hiding this comment

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

LGTM

@Salakar Salakar changed the title Fix logging issue. fix: issue with logging Sep 11, 2023
@Salakar Salakar merged commit dba043c into firebase:main Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants