-
Notifications
You must be signed in to change notification settings - Fork 200
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 for #1055 #1056
Fix for #1055 #1056
Conversation
b828ee8
to
9eec4a4
Compare
@taeold all sorted. Added extra couple of test cases too. I've pulled out the regex from the function too - I couldn't see any good reason for it being a local variable but can change if you'd prefer it left as it was! |
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.
Thank you!
Can you add a changelog entry? Something like
Fixes bug where some RTDB instance names were incorrectly parsed (#1056).
it('should return the correct instance and path strings if root path is /refs', () => { | ||
const [instance, path] = database.extractInstanceAndPath( | ||
'projects/_/instances/foo/refs/refs' | ||
); | ||
expect(instance).to.equal('https://foo.firebaseio.com'); | ||
expect(path).to.equal('/refs'); | ||
}); | ||
|
||
it('should return the correct instance and path strings if a child path contain /refs', () => { | ||
const [instance, path] = database.extractInstanceAndPath( | ||
'projects/_/instances/foo/refs/root/refs' | ||
); | ||
expect(instance).to.equal('https://foo.firebaseio.com'); | ||
expect(path).to.equal('/root/refs'); | ||
}); | ||
|
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.
❤️ Thank you for adding tests!
src/providers/database.ts
Outdated
@@ -307,6 +307,8 @@ export class RefBuilder { | |||
}; | |||
} | |||
|
|||
const resourceRegex = /^projects\/([^/]+)\/instances\/([a-zA-Z0-9-]+?)\/refs(\/.+)?/; |
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.
I'm not convinced (yet!) that we need non-greedy operator on the capturing group for instance based on the provided test cases:
e.g.
const greedy = /^projects\/([^/]+)\/instances\/([a-zA-Z0-9-]+)\/refs(\/.+)?/
const nongreedy = /^projects\/([^/]+)\/instances\/([a-zA-Z0-9-]+?)\/refs(\/.+)?/
console.log("projects/_/instances/foo/refs/root/refs".match(greedy)[2]) // => foo
console.log("projects/_/instances/foo/refs/root/refs".match(nongreedy)[2]) // => foo
console.log("projects/_/instances/foo/refs/refs".match(greedy)[2]) // => foo
console.log("projects/_/instances/foo/refs/refs".match(nongreedy)[2]) // => foo
Can you share edge cases that require non-greedy operator?
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.
Yeh I think you're right, now we've taken out the /
from the set in the instances capture group the non-greediness is no longer needed - shall update 👍
@taeold done - regex changed back to greedy and change log entry added 👍 |
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.
🚀
Thanks @taeold for the quick turn around, any ideas on an ETA for a release? |
@rhodgkins We have a release scheduled next Monday! Will keep you posted. |
@taeold any updates - or by "next Monday" you meant the 21st? (Sorry for being persistent but this is a big issue for us as we've had a user created with their |
@rhodgkins Sorry for the delay. This change was released today! |
Fixes #1055