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(NODE-4423): better type support for nested objects in query & update #3328

Merged
merged 7 commits into from
Jul 20, 2022

Conversation

coyotte508
Copy link
Contributor

@coyotte508 coyotte508 commented Jul 20, 2022

Description

What is changing?

Better typing for update & query

Is there new documentation needed for these changes?

What is the motivation for this change?

	interface User {
		pet: {
			name: {
				first: string;
				last: string;
			};
		};
	}
	
	// This works when it shouldn't
	const query: mongodb.Filter<User> = { "pet.name": { first: 1 } };
	// This doesn't work when it should
	const update: mongodb.UpdateFilter<User> = { $set: { "pet.name": { first: "a", last: "b" } } };

The second error is a regression. This PR fixes both issues

Tickets:

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@coyotte508 coyotte508 changed the title <type>(NODE-4423)<!>: better type support for nested _objects_ in query & update fix(NODE-4423): better type support for nested **objects** in query & update Jul 20, 2022
@dariakp dariakp added the External Submission PR submitted from outside the team label Jul 20, 2022
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Thanks for the submission! The tests look solid, I'm adding a few clean up items and will revisit once this passes our CI.

test/types/community/collection/filterQuery.test-d.ts Outdated Show resolved Hide resolved
test/types/community/collection/filterQuery.test-d.ts Outdated Show resolved Hide resolved
test/types/community/collection/filterQuery.test-d.ts Outdated Show resolved Hide resolved
@dariakp dariakp added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jul 20, 2022
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

lint fix

test/types/community/collection/updateX.test-d.ts Outdated Show resolved Hide resolved
@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jul 20, 2022
@dariakp dariakp changed the title fix(NODE-4423): better type support for nested **objects** in query & update fix(NODE-4423): better type support for nested objects in query & update Jul 20, 2022
@dariakp dariakp merged commit 05e007b into mongodb:main Jul 20, 2022
@coyotte508
Copy link
Contributor Author

PS: Would it be possible after this and the previous PR #3259 to get a mention in the release notes for 4.8 and / or be added to https://github.com/mongodb/node-mongodb-native/blob/main/CONTRIBUTORS.md ?

All the best!

@dariakp
Copy link
Contributor

dariakp commented Jul 20, 2022

@coyotte508 Apologies for the oversight on the release notes - we always want to give credit to the community for the contributions, so I'll get that fixed right away. The contributors file is currently not maintained, but we have plans to start generating it from github data.

@jaydenseric
Copy link

When is this going to be published? I can't use v4.8 it seems due to the problems this PR intends to fix:

Screen Shot 2022-07-23 at 3 55 38 pm

@coyotte508 coyotte508 deleted the fix-update-typing branch July 23, 2022 12:26
@dariakp
Copy link
Contributor

dariakp commented Jul 25, 2022

@jaydenseric We're looking to put out a patch tomorrow!

@jer-sen
Copy link
Contributor

jer-sen commented Aug 5, 2022

@coyotte508 this kind of updates fail type check:

{ $set: { 'aaa.$.bbb': "ccc" } }

{ $set: { 'aaa.$[].bbb': "ccc" } }

{ $set: { 'aaa.$[ddd].bbb': "ccc" } } (arrayFilters option)

Any idea how to fix?

@coyotte508
Copy link
Contributor Author

@jer-sen for me there are no TS errors, but it's more lax (allows invalid types).

@jer-sen
Copy link
Contributor

jer-sen commented Aug 5, 2022

@coyotte508 I also have an issue with variables in keys:

{ $set: { [`aaa.${1 + 1}`]: "bbb" } }

It seems that TS give to { [`aaa.${1 + 1}`]: "bbb" } the type Record<string, 'bbb'>.

Any idea how to fix this without a forced cast?

@jer-sen
Copy link
Contributor

jer-sen commented Aug 5, 2022

@jer-sen for me there are no TS errors, but it's more lax (allows invalid types).

I think it's because I use readonly arrays:

const _: MatchKeysAndValues<{ aaa: { bbb: 'ccc' }[] }> = { 'aaa.$.bbb': 'ccc' }; is ok
const _: MatchKeysAndValues<{ aaa: readonly { bbb: 'ccc' }[] }> = { 'aaa.$.bbb': 'ccc' }; is not ok

Could you improve your PR to handle this case?

@coyotte508
Copy link
Contributor Author

I think https://github.com/mongodb/node-mongodb-native/blob/main/src/mongo_types.ts#L532 is the problem, you could change [Key, ...NestedPaths<Type[Key]>] to [Key, ...NestedPaths<Type[Key]>] | [Key] like https://github.com/mongodb/node-mongodb-native/blob/main/src/mongo_types.ts#L534 two lines below.

You can do the PR if this changes fixes your issue :), or open an issue for someone of the mongodb team on JIRA.

I'm going on holiday myself!

@jer-sen
Copy link
Contributor

jer-sen commented Aug 5, 2022

Thanks @coyotte508 but I am a bit lost...
@dariakp maybe you could have a look?

@jer-sen
Copy link
Contributor

jer-sen commented Aug 8, 2022

@coyotte508 @dariakp I have created:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Submission PR submitted from outside the team Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants