blob.go: adapt code to use helpers for paths

[jf]

No description provided.

[ahmetb]

I actually have a better plan. Do you mind creating methods like pathForContainer and pathForBlob then we can get rid of all those duplication of fmt.Sprintfs and have the path logic only in one place?

[jf]

Well now that is better alright!

[jf]

@ahmetalpbalkan are you ok with these methods being called, and the results used directly without any intermediate "path" variable? There are no error returns with fmt.Sprintf; it always returns a string.

so eg.

uri := b.client.getEndpoint(blobServiceName, b.pathForBlob(container, name), url.Values{})

instead of

path := fmt.Sprintf("%s/%s", container, name)
uri := b.client.getEndpoint(blobServiceName, path, url.Values{})

Also, what would you have these be methods of? StorageClient? BlobStorageClient? or should they be generic methods, and be put in util.go?

[ahmetb]

yeah I'm ok (as long as there's nothing complicated on that line except calling pathForBlob), these can go to blob.go as global non-exported functions. there's no need to make it functions of BlobStorageClient.

oh also, unit tests for those can go to blob_test.go

[jf]

Actually we dont need a pathForContainer, do we? Just pathForBlob.

[ahmetb]

you actually do. what happens when tomorrow we want to address containers as /{container} in the code? :) you need to change 20 different places instead of 1. that logic must be only in one place and it should be testable. that's why these helpers are crucial.

[jf]

eh... well I guess you're right. I was focussed on the present. :}

[jf]

@ahmetalpbalkan , this is what I have so far (minus the tests). Let me know what you think.

Note: I've added a / at the front of the constructed paths because that's what will happen anyway (see http://golang.org/src/net/url/url.go?s=12230:12259#L452, line 481). I'm shortcutting the code, and making things clearer imo.

(And of course, the current tests pass with this code).

[ahmetb]

Nope please don't add that. That duplicates the job of net/url package and might have introduced bugs elsewhere. I wouldn't do that unless it's very necessary. (Did you also run the test suite with this change?)

[jf]

Yes I did (added that in my comment after I first posted it). I don't think it really duplicates the work of the code in net/url, though. I see that line (in net/url as I pointed) as just a precaution, or "helper" of sorts ("if the slash isn't prepended, we'll prepend it").

Paths are after all meant to start with a /, right? And I would prefer to make that clear, because I was wondering that myself when I first came to this code. I had to go dig into the code for getEndpoint(), and then net/url's "url.String" before I understood that it was ok not to have that / at the front.

[ahmetb]

Okay I guess it's okay unless it makes requests with double slashes (//) or generates GetBlobURL or SAS URL like that. These are already covered by the tests. I will take a final look and then merge. Thanks @jf.

[ahmetb]

Oh and I realized pathForBlob can reuse pathForContainer. 😄 also having simple unit tests would be really great. Thanks.

[jf]

ha, I never really considered that! Do you want to keep them separate, though? or are we worrying too much?

I'll get some tests in.

[jf]

ok I've just added the tests. Had to figure out (took some troubleshooting!) why no matter what I did, the tests I added weren't running. So it turns out I found a bug in blob_test.go, and fixed that too. I'm choosing to keep the fix in a separate commit, because I think it's important enough to point out. Prior to this, we weren't running the test for blobSASStringToSign() (and it was wrong to boot, too), apparently!

[ahmetb]

pathForBlob

[jf]

:}

[jeffmendoza]

@jf can you sign a CLA at https://cla.msopentech.com/ ?

[ahmetb]

Please rebase on top of master to include the CI commit, otherwise test will fail.

[jf]

@jeffmendoza is there a more specific privacy policy for developers/open source contributors? I don't see how getting my address, for example, is necessary

[jeffmendoza]

Looks like there is a link at the end of the page to here: https://msopentech.com/privacy-policy/

[jf]

I'm aware and have already looked through it. That's a generic one for all of "MS Open Tech" - "Msopentech.com and other Microsoft Open Technologies, Inc. (“MS Open Tech”) websites and services of MS Open Tech that collect data and display these terms, as well as its offline product support services.". Perhaps some of these will have a legitimate reason for asking for an address.... but as a "pure github contributor and nothing else", I see no reason for Microsoft to ask for my address.

[jf]

Anyway, I've filled in the form. But haven't received any email yet. Is it automated? or manual? (so ie. a delay is normal)

[ahmetb]

@hopetobelievein is CLA approved for @jf ?

[jeffmendoza]

@jf Looks like our new system just took over, can you try: https://cla2.msopentech.com/

[jf]

Ok I'm going to give it one more try. But if this doesn't go through, I'm out for submitting PRs.

[ahmetb]

@jeffmendoza is CLA approved for @jf?

[jf]

I am approved already, @ahmetalpbalkan . But dunno if this is good enough for you, or if you're required to hear from one of the folks you've asked first (in which case, we need to wait on them).

[ahmetb]

@jf ah okay, it is just I don't have access to the interface that shows if you're approved or not. I did not realize you got the approval email from the context above.

[jeffmendoza]

@jf Thanks for bearing with us