-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix(firestore): normalize Firestore pointer path #2929
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(firestore): normalize Firestore pointer path #2929
Conversation
Thanks for that, really good catch! |
Not sure if I should update the CHANGELOG or version as a version wasn't yet uploaded? |
Should be ok (cc @Salakar) - we haven't cut a dev release yet and it's not a user facing change so should be good. |
Ye as above, no need to worry about those, though if the behaviour was also incorrect on the previous Firestore before the rework then feel free to add it to the existing change log for the rework. |
@Salakar well, tested it on an older version, and it didn't work there either. So I'll add it to the CHANGELOG. :) I hope I'll do it correctly :D |
Additional note: With the implementation in the PR calling But I think is more in line with how the native libraries handle it. :) print(Firestore.instance.collection('test').path);
print(Firestore.instance.collection('/test').path);
final snap = await Firestore.instance.document('/some/doc').get();
final ref = snap.data['referenceToOtherDoc'] as DocumentReference;
print(ref.path); Printed originally:
And now prints:
Web implementation: console.log(firebase.firestore().collection('test').path);
console.log(firebase.firestore().collection('/test').path); prints out:
So I think we should be fine with the implementation of this PR. |
@Salakar After doing some research of the current behavior I think there should definitely be some entry in the CHANGELOG, as this
Firestore.instance.collection('/test').path == '/test' // Will now be false, instead of true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Equality checking of references is not user input safe as the internal
Pointer
check unnormalized paths.Related Issues
#2928
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?