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

Fix #443 object merge with same value on same level keys #444

Merged
merged 1 commit into from Oct 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
Expand Up @@ -105,6 +105,7 @@ test_examples:
cd example/issues/404_dup_validator_message;pwd;python app.py
cd example/issues/434_setenv;pwd;python app.py --config development dynaconf
cd example/issues/430_same_name;pwd;python app.py
cd example/issues/443_object_merge;pwd;python app.py

test_vault:
# @cd example/vault;pwd;python write.py
Expand Down
6 changes: 5 additions & 1 deletion dynaconf/utils/__init__.py
Expand Up @@ -42,7 +42,11 @@ def object_merge(old, new, unique=False, full_path=None):

for key, value in old.items():

if existing_value is not None and existing_value is value:
if (
existing_value is not None
and key == full_path[-1]
and existing_value is value
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is still buggy as this comparison cannot handle None types.

To improve this we need to create a way to have a unique signature for each value.

):
continue

if key not in new:
Expand Down
53 changes: 53 additions & 0 deletions example/issues/443_object_merge/README.md
@@ -0,0 +1,53 @@
# [bug] Setting values being stripped out after merge

* created by @daviddavis on 2020-10-07 14:13:58 +0000 UTC

**Describe the bug**
I'm seeing where values are being stripped out of settings. This is on dynaconf 3.1.1 with Python 3.7.7.

**To Reproduce**
Run this code:

```python
from dynaconf.utils.boxing import DynaBox
from dynaconf.utils import object_merge

existing_data = DynaBox({"DATABASES": {'default': {'ENGINE': 'django.db.backends.postgresql_psycopg2', 'HOST': 'localhost', 'PASSWORD': 'pulp', 'NAME': 'pulp', 'USER': 'pulp'}}})
new_data = DynaBox({'DATABASES': {'default': {'USER': 'postgres'}}})
split_keys = ['DATABASES', 'default', 'USER']
object_merge(old=existing_data, new=new_data, full_path=split_keys)
```

The output:

```
<Box: {'DATABASES': {'default': {'USER': 'postgres', 'ENGINE': 'django.db.backends.postgresql_psycopg2', 'HOST': 'localhost'}}}>
```

Notice that NAME and PASSWORD keys have been dropped.

**Expected behavior**
I would expect the final settings dict to be:

```
{"DATABASES": {'default': {'ENGINE': 'django.db.backends.postgresql_psycopg2', 'HOST': 'localhost', 'PASSWORD': 'pulp', 'NAME': 'pulp', 'USER': 'postgres'}}}
```

## Comments:

### comment by @rochacbruno on 2020-10-07 14:31:15 +0000 UTC

The bug is in

https://github.com/rochacbruno/dynaconf/blob/8998cac224a985dc8bb783f2a5abd71ff4e6769c/dynaconf/utils/__init__.py#L45

As all the values are `pulp` it is wrongly using `is` to compare and `"pulp" is "pulp"` will always be `True` regardless the key it is in.

```py
>>> s1 = "pulp"
>>> s2 = "pulp"
>>> s1 is s2
True
```

I will need to include the full path to the key on the comparison or change the `DynaBox` in a way that each value gets its own identity even if the value is the same.
25 changes: 25 additions & 0 deletions example/issues/443_object_merge/app.py
@@ -0,0 +1,25 @@
from dynaconf.utils import object_merge
from dynaconf.utils.boxing import DynaBox

existing_data = DynaBox(
{
"DATABASES": {
"default": {
"ENGINE": "django.db.backends.postgresql_psycopg2",
"HOST": "localhost",
"PASSWORD": "pulp",
"NAME": "pulp",
"USER": "pulp",
}
}
}
)
assert existing_data["DATABASES"]["default"]["USER"] == "pulp"

new_data = DynaBox({"DATABASES": {"default": {"USER": "postgres"}}})
split_keys = ["DATABASES", "default", "USER"]
new = object_merge(old=existing_data, new=new_data, full_path=split_keys)

assert new["DATABASES"]["default"]["USER"] == "postgres"
assert new["DATABASES"]["default"]["NAME"] == "pulp"
assert new["DATABASES"]["default"]["PASSWORD"] == "pulp"