Check for null resourceName before attempted to delete a cert #669
Conversation
Without this, the sdk will send a null resourceName to the server, resulting in a somewhat misleading 404 error being returned.
| @@ -175,6 +175,14 @@ describe('admin.projectManagement', () => { | |||
| expect(certs.length).to.equal(0); | |||
| }); | |||
| }); | |||
|
|
|||
| it('add a cert and then remove it fails', () => { | |||
There was a problem hiding this comment.
This is testing the actual behaviour (both pre and post this PR) rather than the intended behaviour. It seems unfortunate that you can't delete a cert you just added (without going through getCerts first).
An alternative would be for the addShaCertificate to return a Promise where X is either the resourceName or the cert itself, populated with the resourceName (rather than void.) But that requires a minor API change.
Another alternative would be to mutate the cert passed to addShaCertificate to populate the resourceName. But that's a bit awkward since (a) mutating parameters is always a little awkward, and (b) it may need to be done asynchronously, so there'd be a timing consideration there too.
There was a problem hiding this comment.
The CreateShaCertificate public API does return a ShaCertificate, so it's totally reasonable to update addShaCertificate to return Promise<ShaCertificate>. I would not recommend mutating the ShaCertificate that was passed to addShaCertificate.
There was a problem hiding this comment.
Returning a Promise sounds like a good idea. And not a breaking change in this case.
| @@ -175,6 +175,14 @@ describe('admin.projectManagement', () => { | |||
| expect(certs.length).to.equal(0); | |||
| }); | |||
| }); | |||
|
|
|||
| it('add a cert and then remove it fails', () => { | |||
There was a problem hiding this comment.
Returning a Promise sounds like a good idea. And not a breaking change in this case.
| @@ -175,6 +175,14 @@ describe('admin.projectManagement', () => { | |||
| expect(certs.length).to.equal(0); | |||
| }); | |||
| }); | |||
|
|
|||
| it('add a cert and then remove it fails', () => { | |||
There was a problem hiding this comment.
The CreateShaCertificate public API does return a ShaCertificate, so it's totally reasonable to update addShaCertificate to return Promise<ShaCertificate>. I would not recommend mutating the ShaCertificate that was passed to addShaCertificate.
| admin.projectManagement().shaCertificate(SHA_256_HASH); | ||
| return androidApp.addShaCertificate(shaCertificate) | ||
| .then(() => androidApp.deleteShaCertificate(shaCertificate)) | ||
| .should.eventually.be.rejected; |
There was a problem hiding this comment.
It would be nice to check that the rejection reason is the new FirebaseError you added, and make the it(...) description of this test indicate why it should fail (ie. no resourceName).
There was a problem hiding this comment.
Strangely, I couldn't get .should.eventually.be.rejectedWith(FirebaseProjectManagementError) to work, so instead I did .rejectedWith('error message here'). The other integration tests seem to do the same.
Without this, the sdk will send a null resourceName to the server,
resulting in a somewhat misleading 404 error being returned.