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

Add test to show that you can't use patternProperties in schema #4951

Merged
merged 5 commits into from
Oct 13, 2023
Merged

Add test to show that you can't use patternProperties in schema #4951

merged 5 commits into from
Oct 13, 2023

Conversation

adam-lynch
Copy link
Contributor

This PR contains:

  • IMPROVED TESTS

Describe the problem you have without this PR

This is for #4926. It shows that if you use patternProperties to define the value of unknown keys, that it'll error due to schema key validation.

@pubkey
Copy link
Owner

pubkey commented Sep 19, 2023

This use case is missing in the dev-mode plugin. It should not regex-check patternProperties.

@adam-lynch
Copy link
Contributor Author

I've just pushed a fix for that but it's still failing (the result includes patternProperties and type)

@adam-lynch
Copy link
Contributor Author

How can we move this forward?

@adam-lynch
Copy link
Contributor Author

I've synced this with master and improved the test so it's more clear

@adam-lynch
Copy link
Contributor Author

Can we please move this forward?

@adam-lynch
Copy link
Contributor Author

Can we please move this forward?

To be clear, I'm willing to put in work on this if needed (as I have done), I just need some direction if so.

@pubkey
Copy link
Owner

pubkey commented Oct 3, 2023

What is the state of this PR?
I think the proposed solution path.split('.').pop() !== 'patternProperties' looks good.
Can you move the test to somehwere else, like the rx-schema tests?

The line that atm fails the CI should be removed. Accessing the properties like that is not possible
assert.deepStrictEqual(Object.keys(myDocument.get('tags')), Object.keys(tags), 'Object.keys(myDocument.get(\'tags\'))');

@adam-lynch
Copy link
Contributor Author

This PR's initial aim was to prove that patternProperties can't be used. Now it has become a PR to fix that.

I fixed the initial regex issue. But in my opinion it still doesn't work because the tags object retrieved has extra keys (patternProperties and type). It seems you disagree with that;

The line that atm fails the CI should be removed. Accessing the properties like that is not possible
assert.deepStrictEqual(Object.keys(myDocument.get('tags')), Object.keys(tags), 'Object.keys(myDocument.get(\'tags\'))');

It seems like you're saying that you can't do Object.keys(...) of myDocument.get('tags'). I don't understand why; it gives the correct data, plus some extra unnecessary stuff from the schema. If it didn't return those extra properties, it would be perfect.

If you're right, then I can remove the lines that break the test and we can merge it, but I wouldn't want to use .get(...) in our project. And generally it would mean that there is no 100% reliable way to get individual properties from an RxDocument; .toJSON()[key] must always be used.

@adam-lynch
Copy link
Contributor Author

@pubkey we're pushing hard to rewrite all of our app to use RxDB and this is a bit of a blocker / an unknown that we'd like to resolve. Could I please get an answer on the above?

@pubkey
Copy link
Owner

pubkey commented Oct 9, 2023

Using .get() on unknown properties will work in the next major RxDB version.
Please remove this line from the test so we can check if everything else works.

@adam-lynch
Copy link
Contributor Author

Using .get() on unknown properties will work in the #4949 (comment) major RxDB version.

Ok I had no idea that applied to the use of patternProperties (as well as additionalProperties); I would have guessed patternProperties aren't "unknown". The point in using patternProperties vs additionalProperties was to give them schema.

I'll update this test.

@adam-lynch
Copy link
Contributor Author

Done

@pubkey pubkey merged commit 0545ae2 into pubkey:master Oct 13, 2023
pubkey added a commit that referenced this pull request Oct 13, 2023
@pubkey
Copy link
Owner

pubkey commented Oct 13, 2023

Thank you for your work. I merged this into the v15 branch.

@adam-lynch
Copy link
Contributor Author

@pubkey do you have a rough idea of when v15 will be released?

@pubkey
Copy link
Owner

pubkey commented Oct 23, 2023

The beta will be release this week. Non-beta release will be at least 4 weeks. Maybe longer depending on which problems occur.

@adam-lynch
Copy link
Contributor Author

@pubkey ok thanks. I assume that means there will be matching rxdb and rxdb-premium versions released.

@pubkey
Copy link
Owner

pubkey commented Oct 23, 2023

Yes, premium will also be released in the matching beta version.

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