Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change "mesage" type hint of logging methods from "str" to "LiteralString" #1206

Open
Delgan opened this issue Sep 15, 2024 · 6 comments
Open
Labels
enhancement Improvement to an already existing feature

Comments

@Delgan
Copy link
Owner

Delgan commented Sep 15, 2024

It should help with catching this kind of problem referenced in the documentation:

SECRET_KEY = 'Y0UC4NTS33Th1S!'

class SomeValue:
    def __init__(self, value):
        self.value = value

# If user types "{value.__init__.__globals__[SECRET_KEY]}" then the secret key is displayed.
message = "[Custom message] " + input()
logger.info(message, value=SomeValue(10))

A type checker would reject such code if message is not a LiteralString.

@Delgan Delgan added the enhancement Improvement to an already existing feature label Sep 15, 2024
@trim21
Copy link
Contributor

trim21 commented Sep 18, 2024

f-string is faster and more readable than implicit format. So some users just use f-string at logging message if they know a message will always be logged, for example, as warning message or error message.

But the problem is, f-string is not always LiteralString, if it contain any element are not LiteralString.

So this will be a type error logger.info(f"update record {record.id} {record.url}"), if all log message typed as LiteralString.

https://docs.python.org/3/library/typing.html#typing.LiteralString

@reneleonhardt
Copy link

It would be great for logging frameworks if simple f-strings could be evaluated lazily by currying all required context variables until __str__ is called... i.e. a factory method "a={a} func={func}".lazy(a=1, func=lambda: 2) 😅

@Delgan
Copy link
Owner Author

Delgan commented Sep 22, 2024

@trim21 You're correct. It would be too much of a burden for users.

I still believe using LiteralString is superior for a few reasons:

  • it reduces the risk of injection attacks
  • it leads to better performance
  • it encourages the use of structured logging

However, Loguru is designed to be flexible, and users have different expectations and habits. It's not possible to impose this.
It would be interesting if we could optionally change the type hint of message, though.

@reneleonhardt Python does not allow this. You need to use an auxiliary function, for example the lazy argument of logger.opt().

@trim21
Copy link
Contributor

trim21 commented Sep 22, 2024

it leads to better performance

why?

I thought this is a typing only change in .pyi file?

@Delgan
Copy link
Owner Author

Delgan commented Sep 22, 2024

I mean, because it avoids formatting the arguments if the log level is too high and that the message won't be logged anyway.

@trim21
Copy link
Contributor

trim21 commented Sep 22, 2024

I mean, because it avoids formatting the arguments if the log level is too high and that the message won't be logged anyway.

Ok, I get it.

But it lead to worse perf for always logged line if f-string can't be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an already existing feature
Projects
None yet
Development

No branches or pull requests

3 participants