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(postgrest): Update parameter type of match() filter so that null values cannot be passed. #919

Merged
merged 3 commits into from
May 13, 2024

Conversation

dshukertjr
Copy link
Member

What kind of change does this PR introduce?

Fixes #917

What is the current behavior?

Currently, users can pass null to match() filter.

What is the new behavior?

Users will not be able to pass null to match().

@dshukertjr dshukertjr changed the title fix: match does not accept null value fix: Update the parameter type of match() to not accept null values. May 7, 2024
@Vinzent03
Copy link
Collaborator

This is technically a breaking change. A map may be of type Map<String,dynamic>, which is not assignable to Map<String, Object>. For example, if you type Map a = {"a": 4,"b":"String"} it's not assignable.

@dshukertjr
Copy link
Member Author

I think we can still consider it a fix for a type that we didn't get right in the first place.

@Vinzent03
Copy link
Collaborator

But the type of a map may be easily Map<dynamic,dynamic> despite having no null entries. So a working code might break now. I would prefer to note it in the documentation and use an assert over the actual values instead.

@dshukertjr
Copy link
Member Author

dshukertjr commented May 7, 2024

Fair enough 👍

@dshukertjr dshukertjr changed the title fix: Update the parameter type of match() to not accept null values. fix(postgrest): Add assertion to ensure no null value is passed to match() filter. May 7, 2024
var url = _url;
query.forEach((k, v) => url = appendSearchParams('$k', 'eq.$v', url));
query.forEach((k, v) => url = appendSearchParams(k, 'eq.$v', url));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be good to undo this.

Suggested change
query.forEach((k, v) => url = appendSearchParams(k, 'eq.$v', url));
query.forEach((k, v) => url = appendSearchParams('$k', 'eq.$v', url));

@bdlukaa
Copy link
Collaborator

bdlukaa commented May 7, 2024

I like the first proposed solution better. It gives the developers type-safety, causing less hassle. It might be good to have this shipped in the next major release.

@dshukertjr
Copy link
Member Author

I like the first proposed solution better. It gives the developers type-safety, causing less hassle.

I like it too. I think sometimes we try to be a bit too picky about what defines a breaking change. I feel like the world does revolve around these minor "technically breaking changes" being shipped as a bug fix all the time. Flutter itself ships "breaking changes" without bumping the major versions all the time, and as long as it's under moderation, I don't think we should hesitate to ship these changes.

I'd say we just ship it with the original change.

@dshukertjr dshukertjr requested a review from Vinzent03 May 8, 2024 03:56
@dshukertjr dshukertjr changed the title fix(postgrest): Add assertion to ensure no null value is passed to match() filter. fix(postgrest): Update parameter type of match() filter so that null values cannot be passed. May 8, 2024
Copy link
Collaborator

@Vinzent03 Vinzent03 left a comment

Choose a reason for hiding this comment

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

I don't think you can compare Flutter breaking changes with package breaking changes. Packages should use semver, Flutter doesn't.
I'm pretty sure three are code basis, which throw a static analysis error now, which definitely is a breaking change. But sure, our other bug fixes may sometimes be more breaking, and they don't even show up when compiling.
I wouldn't make this breaking change, but you decide.

@dshukertjr dshukertjr merged commit 0902124 into main May 13, 2024
10 checks passed
@dshukertjr dshukertjr deleted the fix/match branch May 13, 2024 09:10
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.

Matching on null fails to return anything
3 participants