feat(upload): auto-detect file content-type if not sent

[akrem-chabchoub]

Checklist

• I have read the coding guide.
• My change requires a documentation update, and I have done it.
• I have added tests to cover my changes.
• I have filled out the description and linked the related issues.

Description

For single-file POST /bzz, Content-Type is optional: if the client sends it, we keep it exactly, if not we infer it from the first bytes of the body.

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

#5346

Screenshots (if appropriate):

[acud]

what is the status of this PR?

[akrem-chabchoub]

Still not fully tested yet.
Currently on hold due to other priorities.
Work is expected to resume asap.

[acud]

if (headers.IsDir || isMultipart) && contentTypeHdr == "" {...}

[akrem-chabchoub]

Maybe this is more readable, wdyt ?

isDirUpload := headers.IsDir || isMultipart
if !isDirUpload {
	s.fileUploadHandler(ctx, logger, span, ow, r, putter, headers.Encrypt, tag, headers.RLevel, headers.Act, headers.HistoryAddress)
	return
}

if contentTypeHdr == "" {
	logger.Error(nil, "content-type required for directory upload")
	jsonhttp.BadRequest(w, errInvalidContentType)
	return
}

s.dirUploadHandler(ctx, logger, span, ow, r, putter, headers.Encrypt, tag, headers.RLevel, headers.Act, headers.HistoryAddress)

[acud]

both are fine 👍 the point was to just reduce nesting

[martinconic]

Is this necessary?

[akrem-chabchoub]

Before my changes the dirUploadHandler was accepting contentTypeString (although it is present request).
So I changed that here and did trim and set the new value in main handler.

[acud]

I think that I agree with @martinconic that this is not really clear why this is needed. If you're already reading, cleaning, parsing and checking the content type header - it does not make any more sense to update the value in the Header type.

[akrem-chabchoub]

@acud @martinconic
Good point, i agree this can look a bit strange at first.

i set the header on purpose, so we clean Content-Type one time, and then all next steps read the same value.

without this, one part can use the trimmed value, but another part (fileUploadHandler) can still read the original raw header. this can cause small inconsistent behavior later, especially for multipart handling and metadata.

if changing the header feels too implicit, i can refactor and pass the cleaned value explicitly to the next functions. i am fine with both styles, but i think we should keep one canonical content-type value in one place.

[acud]

Thanks @akrem-chabchoub.
The problem from my perspective is that you're already doing cleaning and checking of a value, mutating it, but then you're telling downstream function calls to potentially do the same, again, because you've removed the argument from the signature. This is from my perspective not necessary and potentially duplicates code.

This may work in middleware style calls where the middlewares are decoupled (and you want to be setting a header on one middleware that processes for example a user-login or something along those lines, providing you with extra details down the call stack), but I can't see how this is needed here (you're already at the last middleware in the API).

I won't block the PR on this, please choose how you'd like to proceed with this.

[martinconic]

The comment is not relevant anymore, right? the header is not validated by the caller. Maybe something like this:

// Parse error is ignored; unsupported media types are caught by the default case below.

, or do we want to validate here?

[akrem-chabchoub]

Good catch, agreed.
I’d keep validation behavior as-is here, since parse failures and unsupported types is handled in the switch case.

[sbackend123]

How would it work on edge cases like:

• empty body

[acud]

Thanks @akrem-chabchoub.
The problem from my perspective is that you're already doing cleaning and checking of a value, mutating it, but then you're telling downstream function calls to potentially do the same, again, because you've removed the argument from the signature. This is from my perspective not necessary and potentially duplicates code.

This may work in middleware style calls where the middlewares are decoupled (and you want to be setting a header on one middleware that processes for example a user-login or something along those lines, providing you with extra details down the call stack), but I can't see how this is needed here (you're already at the last middleware in the API).

I won't block the PR on this, please choose how you'd like to proceed with this.