Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add encoding function that returns Data. Fixes #94 #95

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

andrew804
Copy link

Added encodeToData function to FormDataEncoder which returns Data instead of String.
Added serializeToData function to MultipartSerializer which returns Data instead of String.
Added decode function to FormDataDecoder which accepts Data as input.
Added test for encoding and decoding a jpeg image with the new Data functions.
Added test for encoding and decoding a jpeg image with the original String functions.
Added image.jpeg in /Utilities

This PR fixes #94

@andrew804 andrew804 requested review from 0xTim and gwynne as code owners March 1, 2024 22:23
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A great start! Just a few nits, nothing major.

Sources/MultipartKit/FormDataDecoder/FormDataDecoder.swift Outdated Show resolved Hide resolved
Comment on lines 39 to 41
var buffer = ByteBufferAllocator().buffer(capacity: 0)
try self.serialize(parts: parts, boundary: boundary, into: &buffer)
return Data(buffer.readableBytesView)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import NIOFoundationCompat and use Data(buffer: buffer, byteTransferStrategy: .automatic).

And while you're at it, change the String version to use String(buffer: buffer) 🙂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used ByteBuffer(data: data) instead but assume that’s the same.

Tests/MultipartKitTests/MultipartTests.swift Outdated Show resolved Hide resolved
Sources/MultipartKit/FormDataEncoder/FormDataEncoder.swift Outdated Show resolved Hide resolved
Sources/MultipartKit/FormDataEncoder/FormDataEncoder.swift Outdated Show resolved Hide resolved
@gwynne gwynne added bug Something isn't working semver-minor Contains new API labels Mar 1, 2024
@andrew804
Copy link
Author

Thank you for your fast feedback! Hopefully I covered everything - please let me know if not or if you have any further comments.

@Joannis Joannis requested a review from gwynne March 6, 2024 13:27
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - @gwynne anything else to add?

@0xTim
Copy link
Member

0xTim commented Mar 20, 2024

@andrew804 Looks like this is failing in CI

@andrew804
Copy link
Author

@0xTim the failing test is the one that tests for encoding and decoding a jpeg image with the original string functions which highlights the original bug ie the original bug is still present, using the string encoding and decoding functions still corrupts the data.

Therefore I could either 1) add a warning in the documentation to not use the string functions and instead use the data functions for encoding and decoding images, then just remove the failing test or 2) remove the troublesome string functions entirely and include this PR in the next major release.

Unless you have any other suggestion, what would you prefer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-minor Contains new API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encoding corrupts data
3 participants