MultipartKit V5

[ptoffy]

No description provided.

[adam-fowler]

Some initial comments.

[ptoffy]

@adam-fowler Do you want to take a look at this again and see if stuff makes more sense now? I added in some binary data (which even contains hex-CRLF 😄) to the tests so it should be able to parse anything now

[ptoffy]

@Joannis @simonjbeaumont @czechboy0 pulling you into this so you can take a look if you want

[Joannis]

This makes a copy from string, can't we use withContiguousMemoryIfAvailable?

[ptoffy]

Mhh I don't think that would work any better because we need the Collection there. We can't pass in the raw bytes and if we're copying them to an array we're still making a copy at that point right?

[Joannis]

Since you're doing an .append on the parser you're right that you're already making a copy. Except right now you're making two copies of the same data, and each time you're also allocating space for that data.

[ptoffy]

No, I mean we can't pass the raw pointer to the decode method because it expects the collection of bytes, so this can't be done

try string.utf8.withContiguousStorageIfAvailable { bytes in
    decode(D.self, from: bytes, boundary: boundary)
} ?? decode(D.self, from: Array(string.utf8), boundary: boundary)

And if we were to do something like

if let bytes = string.utf8.withContiguousStorageIfAvailable(Array.init) {
    try decode(D.self, from: bytes, boundary: boundary)
} else {
    try decode(D.self, from: Array(string.utf8), boundary: boundary)
}

we're still initialising an array with the raw bytes so I think this doesn't really save us a copy.
Unless you mean a different way of using withContiguousStorageIfAvailable

[Joannis]

I would really prefer not copying unnecessarily in a library like this

[ptoffy]

I agree with the proposed changes but I moved the nextCollatedPart() to simply be the next() method of the new sequence which makes more sense to me

[0xTim]

Nothing blocking on my end. Once other comments have been resolved we can look at integrating this into projects to see how it actually works

[Joannis]

My main (only) issue is that this library makes a lot of copies out of every byte carrier (String, Array etc)

[gwynne]

Various nits

[Joannis]

We should still optimise the one copy, but I'm happy with this!

[ptoffy]

Cool - if @adam-fowler has no outstanding requests then this should be ready to go

[adam-fowler]

I agree with the proposed changes but I moved the nextCollatedPart() to simply be the next() method of the new sequence which makes more sense to me

While I can see the sense in this, I'm thinking of how this may be used. Given multipart files can come with sections of different structure, some you might want to stream and some you might want collated I think you should re-instate the nextCollatedPart().

One other thing to consider. Nobody is ever going to want to stream headers, so collating them automatically makes sense.

I am trying this out with my swift package registry example just now. Will report back. To give you some context SwiftPM packages a file into a Multipart file consisting of headers for metadata, a metadata block, a headers for the package zip, and then the zip file of the package. I don't want to stream the first three sections but will want to stream the last.

[adam-fowler]

Yeah by making me deal with streaming all the sections it is painful. Would be great if I could do

let multipartStream = StreamingMultipartParserAsyncSequence(...)
let iterator = multipartStream.makeAsyncIterator()
while let part = try await iterator.next() {
    guard case .headerFields(let headers) else { continue }
    // is metadata
    if getParameter(headers[.contentDisposition].first, parameter: "name") == "metadata" {
        guard let metadata = iterator.nextCollatedBlock() else { throw Error() }
        parseMetadata(metadata)
    }
    // is archive
    if getParameter(headers[.contentDisposition].first, parameter: "name") == "archive" {
        while case .bodyChunk(let buffer) = try await iterator.next() {
            streamArchivePartToDisk(buffer)
        }
    }
}

[0xTim]

I think that's a reasonable request for the API. If you're happy @adam-fowler I think we should merge as is and then add the improved API going forward?

[ptoffy]

@0xTim I think adding the change in this PR makes sense. I'll add it in this afternoon and then we can merge

[ptoffy]

@adam-fowler I updated the code to work with your example and I can confirm this works

let stream = makeParsingStream(for: message)
var iterator = StreamingMultipartParserAsyncSequence(boundary: boundary, buffer: stream).makeAsyncIterator()

var accumulatedBody: [UInt8] = []

while let part = try await iterator.next() {
    guard case .headerFields(let fields) = part else { continue }

    if fields.getParameter(.contentDisposition, "name") == "id" {
        guard let id = try await iterator.nextCollatedPart() else {
            throw MultipartMessageError.unexpectedEndOfFile
        }
        #expect(id == .bodyChunk(ArraySlice("123e4567-e89b-12d3-a456-426655440000".utf8)))
    } else {
        while case .bodyChunk(let chunk) = try await iterator.next() {
            accumulatedBody.append(contentsOf: chunk)
        }
    }
}

#expect(accumulatedBody == pngData)

though I haven't added the test to the branch because I'm a bit reluctant in making fields.getParameter() public. We can always do that later if needed. If you're happy with this I'll merge now