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

Allow middleware hooks to return objects that contained field names that contain dotted strings #1130

Merged
merged 10 commits into from
Sep 29, 2020
Merged

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Sep 25, 2020

The setByKeyPath utility function interprets dots (.) within strings as path separators, and so the string a.b relates to an object of the format {"a": {"b": ...}}.

In many cases this is the desired behaviour - the object being persisted may contain nested properties, and Dexie provides dotted notation as a way to address those paths. However, sometimes objects may have literal property names that contain dots, and it would be useful to provide the ability to address those properties too.

Previously, if middleware hooks returned an object with keys that contained dots, those keys would always have been interpreted as paths, and the corresponding database record(s) would be transformed into nested objects.

This change adds a check to determine whether keys exist as native properties on the target object, before intepreting them as key paths.

Resolves #966

@jayaddison
Copy link
Contributor Author

Question: should a similar change apply on line 80 of the same file too? (related to add or put that generates a create operation)

@dfahlander
Copy link
Collaborator

dfahlander commented Sep 25, 2020

Just to be certain nothing is breaking here, I just published a branch that adds another unit test. Please merge that branch into yours to verify your changes doesn't break existing contract of what modifications are expected to be in the docs for updating hook. The branch is https://github.com/dfahlander/Dexie.js/tree/additional-test-for-1130.

@dfahlander
Copy link
Collaborator

You could also just past the test into your fork if it's simpler:

The test should be shown in the following diff:

https://github.com/dfahlander/Dexie.js/compare/additional-test-for-1130

@jayaddison
Copy link
Contributor Author

The updated test is likely to fail against the change as-is (a good thing, since it demonstrates a gap / problem), I think. I'll take a little more time to fully grok the function contract definition in the docs soon and then take another go.

@jayaddison
Copy link
Contributor Author

On second thoughts, I don't know whether setByKeyPath is the correct place to fix this. It doesn't feel like this change would be easy to make symmetrical with getByKeyPath, and in general it makes the intent and behaviour of dotted key path notation less clear (i.e. sometimes a dotted path can mean a literal key, and sometimes it can mean a nested object path -- and it's context-dependent at runtime).

It might make more sense to look at the getObjectDiff utility function and see whether this should return a nested diff object structure instead of attempting to transform nested fields into dotted key path notation.

There could then be a corresponding applyObjectDiff function that would recursively apply the nested diff to an existing object.

@dfahlander
Copy link
Collaborator

Thanks for the updated commits. You're right it would be more risky to change the behaviour of setByKeyPath(). The use case with dots in keys were not really thought of before. I think the current solution you have would be just an improvement and not risky. I will have another look later and probably merge it in soon.

@dfahlander dfahlander merged commit b2335f9 into dexie:master Sep 29, 2020
@jayaddison jayaddison deleted the fixes/issue-966 branch September 30, 2020 00:16
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.

Updating hooks break when keys have a ..
2 participants