Bugfix: prepare_map returning non-map works correctly + small refactor.#35
Conversation
- Replaced inline function for `prepare_map` with `Function.identity/1` for better performance. - Consolidated logic in `do_diff` to handle type changes and map preparation more efficiently. - Updated path joining to use `join_key/2` for handling nil values. - Added a test case for handling type changes in map values.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the prepare_map function returning non-map values didn't work correctly when diffing maps, and includes performance and code clarity improvements.
- Fixed handling of type changes when
prepare_maptransforms map values to non-map types - Replaced inline anonymous function with
Function.identity/1for better performance - Consolidated path building logic with a new
join_key/2helper function
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/jsonpatch.ex | Fixed prepare_map type change handling, consolidated diff logic, and added path building helper |
| test/jsonpatch_test.exs | Added test case for prepare_map transforming Date structs to strings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| defp do_diff(dest, source, path, key, patches, opts) when are_unequal_lists(dest, source) do | ||
| # uneqal lists, let's use a specialized function for that |
There was a problem hiding this comment.
Fix typo in comment: 'uneqal' should be 'unequal'.
| defp do_diff(dest, source, path, key, patches, opts) when are_unequal_lists(dest, source) do | |
| # uneqal lists, let's use a specialized function for that | |
| # unequal lists, let's use a specialized function for that |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where the prepare_map function could return non-map values, causing type changes to not be handled correctly during diff generation. It also includes performance improvements and code refactoring.
- Replaced inline
prepare_mapfunction withFunction.identity/1for better performance - Consolidated diff logic to handle type changes after map preparation more efficiently
- Refactored path joining logic using a new
join_key/2helper function
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/jsonpatch.ex | Core bug fix and refactoring - handles type changes from prepare_map, consolidates diff logic, and adds join_key/2 helper |
| test/jsonpatch_test.exs | Adds test case demonstrating the fix for type changes via prepare_map (Date to string conversion) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| defp do_list_diff(destination, source, ancestor_path, patches, opts) do | ||
| if opts[:object_hash] do | ||
| do_hash_list_diff(destination, source, ancestor_path, patches, opts) | ||
| else | ||
| do_pairwise_list_diff(destination, source, ancestor_path, patches, idx, opts) | ||
| do_pairwise_list_diff(destination, source, ancestor_path, patches, 0, opts) | ||
| end | ||
| catch | ||
| # happens if we've got a nil hash or we tried to hash a non-map | ||
| :hash_not_implemented -> | ||
| do_pairwise_list_diff(destination, source, ancestor_path, patches, idx, opts) | ||
| do_pairwise_list_diff(destination, source, ancestor_path, patches, 0, opts) |
There was a problem hiding this comment.
The function signature has changed to remove the idx parameter, but the hardcoded 0 values in lines 288 and 293 suggest this parameter is still needed. Consider adding the idx parameter back or ensuring the called functions can handle starting from index 0 correctly.
| do_diff(destination, source, opts[:ancestor_path], nil, [], opts) | ||
| end | ||
|
|
||
| defguardp are_unequal_maps(val1, val2) when val1 != val2 and is_map(val2) and is_map(val1) | ||
| defguardp are_unequal_lists(val1, val2) when val1 != val2 and is_list(val2) and is_list(val1) | ||
|
|
||
| defp do_diff(dest, source, path, key, patches, opts) when are_unequal_lists(dest, source) do | ||
| # uneqal lists, let's use a specialized function for that | ||
| do_list_diff(dest, source, "#{path}/#{escape(key)}", patches, 0, opts) | ||
| do_list_diff(dest, source, join_key(path, key), patches, opts) | ||
| end | ||
|
|
||
| defp do_diff(dest, source, path, key, patches, opts) when are_unequal_maps(dest, source) do | ||
| # uneqal maps, let's use a specialized function for that | ||
| do_map_diff(dest, source, "#{path}/#{escape(key)}", patches, opts) | ||
| # Convert structs to maps if prepare_map function is provided | ||
| dest = maybe_prepare_map(dest, opts) | ||
| source = maybe_prepare_map(source, opts) | ||
|
|
||
| if not is_map(dest) or not is_map(source) do | ||
| # type changed, let's process it again | ||
| do_diff(dest, source, path, key, patches, opts) | ||
| else | ||
| # uneqal maps, let's use a specialized function for that | ||
| do_map_diff(dest, source, join_key(path, key), patches, opts) | ||
| end | ||
| end | ||
|
|
||
| defp do_diff(dest, source, path, key, patches, opts) when dest != source do | ||
| # scalar values or change of type (map -> list etc), let's just make a replace patch | ||
| value = maybe_prepare_map(dest, opts) | ||
| [%{op: "replace", path: "#{path}/#{escape(key)}", value: value} | patches] | ||
| [%{op: "replace", path: join_key(path, key), value: maybe_prepare_map(dest, opts)} | patches] | ||
| end | ||
|
|
||
| defp do_diff(_dest, _source, _path, _key, patches, _opts) do |
There was a problem hiding this comment.
This recursive call to do_diff could potentially cause infinite recursion if prepare_map consistently returns non-map values. Consider adding a guard or depth limit to prevent stack overflow.
|
Hey @Valian . 2.3.1 was released. :-) |
|
Amazing! Thank you :) PS. Tags for 2.3.0 and 2.3.1 seem to be missing on GH, that's why I missed that 2.3.0 was released. |
|
Hups, thx for the hint. I fixed it. |
2 participants
@corka149 I think it's the last PR I'm submitting.
The issue I encountered was with changing type of maps via prepare_map - eg Date or DateTime to strings. This bugfix should fix it, and also made a small refactor to simplify some parts of the code (mostly escaping of paths).
I would greatly appreciate if you could release a new version. Immediately after I'll use it as a dependency in my project 🤗
prepare_mapwithFunction.identity/1for better performance.do_diffto handle type changes and map preparation more efficiently.join_key/2for handling nil values.