But you still haven’t answered the question. Why is a non-literal argument “potentially incorrect”? After all, indent="8" is just as (potentially) incorrect, but won’t get flagged.
And as @storchaka noted, valid use cases like a json.dumps wrapper which passes user-supplied arguments (including indent) on from the caller to dumps will be flagged as (potentially) incorrect, when they clearly aren’t.
Maybe it won’t cause much harm because most uses won’t be affected. But that’s not the question - the question is what actual benefit will it provide? Not just handwavy “might be incorrect” claims, do you (or the OP) have real examples of code that has failed because a non-literal has been passed to indent?
And SQL injection has real, documented cases of harm being caused as a result. That’s what I’m asking for here, and I’m hearing nothing.
I didn’t say they weren’t possible, just that they would be flagged. And “of course” it’s possible to suppress the warning. But suppression is a cost (code needs to change), so all I’m asking is what’s the benefit to offset the cost?
You seem to be convinced that “potentially dangerous” with no explicit examples of the danger is enough justification. I disagree. I doubt either of us will persuade the other. So I think we should drop the discussion. I’m personally pretty sure that your proposal won’t get adopted as it stands, but if it does, then so be it, you were right and I was wrong. It’s not like I can’t make mistakes
Isn’t this kind of things better served by flake8-bandit and the ruff equivalent ? It’s better to rely on these tools than introduce false positive warnings in Python.
def wrap_dumps(..., indent: str | int | None) -> str:
return json.dumps(..., indent="\t" if indent == 8 else indent)
then the definition of wrap_dumps needs to change to avoid errors[1].
As I say, you’re not going to convince me, and I’m not going to convince you. Let’s drop this.
And just to make things worse, the behaviour appears to differ between type checkers - in my tests, mypy doesn’t seem to flag passing a str to a parameter annotated as LiteralString↩︎
I think it comes off more rudely than you likely intended for your PR description to talk about how the negative feedback is from people not understanding how LiteralString works. While there is certainly misunderstanding here, I’m not sure it’s productive to just dismiss all concerns where there’s any misunderstanding.
The opinions do not converge. Could we just add one sentence to the docs? A small warning that improper argument values compromise the validity of JSONEncoder output. Most of you find it fully obvious, but it isn’t. In many other cases improper arguments trigger exceptions.
I would prefer if tools in the standard library didnt produce garbage output and rejected garbage input. There are a lot of cases where people have written things off as “garbage in, garbage out” that later turn out to be abusable in some way.
a json encoder shouldn’t need to care about breaking people using it for things other than encoding json.
I don’t think that’s the whole story here, though. People have demonstrated real use cases for things like indent="| ". Working out precisely what constitutes “garbage input” is far harder than banning everything but whitespace.
Agreed, but the indent parameter isn’t about encoding. It’s explicitly documented as being for pretty printing. And pretty-printed JSON doesn’t need to be valid JSON.
Maybe it would have been better to have a distinct, dedicated pretty-printing function. But that’s not the situation we’re in now, and we can’t simply retroactively redefine the purpose of the indent parameter.
The example that says “pretty print” uses a number, and demonstrates it in contrast to the most compact representation, and the documentation only provides examples that produce valid json.
I can’t read the choice of words “pretty print” to mean “we intended to support an encoder creating invalid representations”, it seems obvious that the intent is about layout, not creating non-json outputs.
The functions that take an indent parameter, as well as the encoder class’s methods use language that indicates the result is intended to be valid json, while there is a section on caveats to that, it does not include this use of indent, and only covers things like inf and invalid unicode sequences.
Don’t forget, the default settings are technically invalid (allowing infinity and nan). The JSON module, like the rest of Python, values practicality over purity.
I ran into this in production years ago. The mistake was mine: I passed a variable as indent without checking it properly. I don’t remember the exact value or have the original code anymore, but I remember spending time tracking down inconsistencies because the output still parsed successfully.
I didn’t bring it up at the time. A similar issue came up recently, which prompted me to investigate this behavior more carefully and open this discussion. I can’t say how common this is for others, but I’ve encountered it more than once in my own work.
I thought indent only controlled how the output was formatted. When I noticed the values were different, I didn’t initially suspect that argument.
Your CLI example made me wonder whether an opt-in strict mode could be useful in json.dumps() as well.
Rather than changing the default, could something like strict=True reject indentation containing characters other than JSON whitespace? That would preserve the existing formatting uses while letting applications catch these mistakes before saving or sending the output.
Unlike checking whether the output parses, this would also catch the cases where indentation changes numeric values but leaves the JSON valid.
The name and scope would need discussion, especially given the points raised about allow_nan and separators. Would this be worth exploring, or is the extra API complexity hard to justify compared with application-side validation?
IMO json should not be modified. If something should change, I would recommend modifying the documentation to mention that passing a string with characters other than spaces to indent produces invalid JSON. I suppose that “spaces” here is limited to tabs ('\t') and ASCII space (' '), and that other “Unicode” space characters (such as '\u1680') also produce invalid JSON.
I would recommend modifying the documentation to mention that passing a string with characters other than spaces and tabs to indent runs the risk of producing invalid JSON or even parsable JSON with incorrect values.
Assuming that spaces, tabs, and any mixture of tabs and spaces, are guaranteed to work.
I don’t think this is actually necessary in most cases, but given the many types of things we accept for indent: non-empty str, positive integers, 0/negative integers/"", None, it isn’t hard to conceive how users might find it confusing.
Perhaps we can add a json.sanitize_indent(indent)utility function that returns the original value or raises ValueError if the indent would result in either invalid JSON or injections?
