Skip to content

fix: prevent mutation of original dict when record.msg is a dict - #66

Open
gaoflow wants to merge 2 commits into
nhairs:mainfrom
gaoflow:fix/dict-msg-mutation
Open

fix: prevent mutation of original dict when record.msg is a dict#66
gaoflow wants to merge 2 commits into
nhairs:mainfrom
gaoflow:fix/dict-msg-mutation

Conversation

@gaoflow

@gaoflow gaoflow commented Jun 29, 2026

Copy link
Copy Markdown

Summary

When record.msg is a dict (used as a structured log message), the format() method assigns it to message_dict by reference. Subsequent mutations to message_dict -- such as injecting exc_info or stack_info -- leak into the caller's original dict, causing unexpected side effects.

Reproduction

import logging
from pythonjsonlogger import jsonlogger

logger = logging.getLogger("test")
handler = logging.StreamHandler()
handler.setFormatter(jsonlogger.JsonFormatter())
logger.addHandler(handler)

data = {"key": "value"}
try:
    1 / 0
except:
    logger.exception(data)

# data now contains "exc_info" -- the original dict was mutated!

Fix

Use record.msg.copy() instead of a direct reference, so the original dict remains untouched.

Tests

All 135 existing tests pass.

(This contribution was made under my direction and I take full responsibility for its correctness.)

When record.msg is used as a dict log message, the format() method
assigned it by reference to message_dict. Subsequent mutations to
message_dict (adding exc_info, stack_info) would leak into the caller's
original dict.

Fix by copying the dict immediately so the original is never modified.
@nhairs

nhairs commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Hi @gaoflow, thanks for putting this together.

Overall it makes sense what it's trying to achieve.

I would however like to take some time to think about if this is a change I want to make. In particular, if my understanding of dict.copy() is correct, this will only create a shallow copy of the data. Meaning that if there is any other mutable object within the msg dict, it's possible that it would also be modified. However I'm not sure if something like deepcopy is robust enough here - stability is the number one priority of this library.

Assuming I decide to go ahead with this change, it would also need a unit test to ensure there are no regressions (the sample provided in the description could form the basis of it). We'd also need consider what documentation / doc-strings need to be updated to make this API contract clear.

Finally, it looks like the failing CI is unrelated to your change.

Covers both directions of the copy behaviour:

- test_log_dict_not_modified pins that exc_info/stack_info are not added
  to the caller's dict.
- test_log_dict_shallow_copied pins that nested values stay shared, so
  swapping in a deep copy is a deliberate change rather than a silent one.

Documents the contract in the format() docstring and the quickstart, and
adds the changelog entry.
@gaoflow

gaoflow commented Jul 27, 2026

Copy link
Copy Markdown
Author

The shallow copy is sufficient here, though not for a comforting reason: the only writes into message_dict are the top-level exc_info and stack_info keys, and both are key rebinds. I instrumented every mutating method on nested dicts/lists inside msg and ran all three formatters across the rename/static/defaults/timestamp/as-array/sequence options — nothing ever wrote through to a nested object. Same when the nested mutable sits under exc_info itself, since {"exc_info": []} is falsy and the key gets overwritten rather than the list touched.

What does remain is that nested values are shared, so the output is a live view of the caller's nested data at serialization time. A deep copy wouldn't close that either — with a buffering handler like MemoryHandler, format() runs after the caller has already mutated, so a format-time copy of any depth is too late. Closing it properly would mean copying at record creation, which seems out of scope for this.

So I documented the contract rather than deepening the copy. 94497f4 adds a regression test built on the sample from the description, a second test asserting nested values stay shared (so a later switch to deepcopy is deliberate rather than silent — happy to drop that one if you'd rather keep the option open), the contract in the format() docstring and the quickstart, plus a changelog entry and the .dev1 bump.

Both tests were mutation-checked: reverting .copy() reddens the first, swapping in deepcopy reddens the second. 200 pass, black/mypy clean.

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.

2 participants