Dowloading a blob should not reject on 304

[arichiardi]

Which service(blob, file, queue, table) does this issue concern?

Blob

Which version of the SDK was used?

10.3.0

What's the Node.js/Browser version?

v8.12.0

What problem was encountered?

When using BlobURL.download and the If-None-Modified modifiedAccessCondition, I noticed that the promise gets rejected with a response code 304.

It feels a bit unnatural though to have to catch and branch in there in this case:

       (.catch (fn [error]
                     (let [code (gobj/get error "statusCode")]
                       (cond
                         ;; not found
                         (= 404 code) (do (when cache
                                            (vswap! cache cache/evict blob-name))
                                          (reject nil))

                         ;; not modified
                         (= 304 code)
                         (resolve (some-> cache
                                          deref
                                          (cache/lookup blob-name)
                                          :content))

                         :else (reject error)))))

Promise rejections are usually signalling unrecoverable problems and are stopping the code flow. I think that 30X would not be treated as exceptions. Even better, maybe an option should allow you to chose what you want to reject on - some library does that.

Steps to reproduce the issue?

Try to pass a populated blobAccessConditions -> modifiedAccessConditions -> ifNoneMatch options to download any blob.

Have you found a mitigation/solution?

See above, not really satisfying.

[XiaoningLiu]

@arichiardi Thanks for your feedback and suggestion about "make an option to chose what want to inject on".

Currently, every Azure Storage REST API defines expected response status codes, such as 200 or 202.
Other response codes will be treated as unexpected, which means something unexpected happens. A promise rejected will happen.

These unexpected responses needs careful attention to explicitly handled, Otherwise, some logical errors will be hidden and may resulting more unexpected behaviors hard to debug.

[arichiardi]

Thank you for answering! Yep that is exactly what happen and what I am debating.

The JS Promise rejections are a replacement of a catch clause that in this and many languages is a kind of error that interrupts the code flow and forces you to handle things, losing context usually.

In summary it should be used sparingly imho and not to control the program flow.

In my understanding a 304 is a normal condition that happens regularly if you cache, not an "exception". That is why it should not interrupt the control flow, aka, I would rather put that if in the .then.

[XiaoningLiu]

@arichiardi Totally understand! Will discuss the details for the injection enhancement, as it's a potential 'big" change covering most of convenience layer methods. I'm afraid we cannot take this in recent changes. Do thanks to your valuable feedback.

We want to have a good balance between "easy coding" and "code quality". The default way about forcing handling all unexpected scenarios helps building up a robust application, but needs more coding efforts.