Fix for #1055 #1056
Fix for #1055 #1056
Conversation
rhodgkins
force-pushed
the
fix-#1055
branch
2 times, most recently
from
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! |
| 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.
❤️ 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.
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]) // => fooCan you share edge cases that require non-greedy operator?
There was a problem hiding this comment.
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 👍 |
|
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