Fix DELETE_FIELD handling: silent no-op for missing fields + nested dict support in set(merge=True) - #5
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: DELETE_FIELD handling improvements
Summary
Two fixes for
DELETE_FIELDbehavior to match real Firestore:1. DELETE_FIELD on non-existent fields should be a silent no-op
update()withfirestore.DELETE_FIELDon a field that doesn't exist is a silent no-op. In mock-firestore, it raised aKeyError.delete_by_pathin_apply_deleteswithtry/except KeyError, matching the pattern already used by_apply_arr_deletesin the same file.2.
set(merge=True)with nested dicts containing DELETE_FIELDset({"stats": {"student123": {"field": DELETE_FIELD}}}, merge=True)deep-merges and deletes the nested field. In mock-firestore, this raised aKeyErrorbecauseapply_transformationsflattened nested dicts into dot-notation keys viaget_document_iterator, then trieddel data[key]using the dot-notation key as a literal top-level key lookup on the still-nested dict.flatten_for_merge()helper that converts nested dicts into dot-notation keys beforeset(merge=True)callsupdate(), so the existing transformation logic works correctly.Motivation
Batch updates that use
set(merge=True)with nested dicts containingDELETE_FIELD(e.g., cleaning up fields that may or may not exist) would crash in tests using mock-firestore, even though they work fine against real Firestore.Changes
mockfirestore/_transformations.pyKeyErrorin_apply_deletes, skip missing fieldsmockfirestore/_helpers.pyflatten_for_merge()to convert nested dicts to dot-notation keysmockfirestore/document.pyflatten_for_merge()inset()whenmerge=Truetests/test_document_reference.pyTest plan
test_document_update_transformerSentinelstill passes (delete existing field)test_document_set_mergeNewValuestill passes (set with merge, flat data)batch.set(ref, {"stats": {"student123": {"field": DELETE_FIELD}}}, merge=True)now works correctly