cyclosproject / ng-openapi-gen

An OpenAPI 3.0 codegen for Angular
MIT License
400 stars 134 forks source link

Breaking change for multipart/form-data #304

Closed nekkon closed 10 months ago

nekkon commented 12 months ago

https://github.com/cyclosproject/ng-openapi-gen/blame/75a957b57f8f80f81313f45c686e97a2f7b91b94/templates/requestBuilder.handlebars#L307

Seems this change converts a stringified object to a binary blob and has broken things for us. Was wondering what is the reason behind this change?

In the Readme it is mentioned that

No data transformation is ever performed before sending / after returning data. This means that a property of type string and format date-time will always be generated as string, not Date. Otherwise every API call would need to have a processing that would traverse the returned object graph before sending the request to replace all date properties by Date. The same applies to sent requests. Such operations are out of scope for ng-openapi-gen

Isn't converting an object/string to a blob/binary against what is mentioned?

Thanks in advance.

karsa-mistmere commented 11 months ago

Hey @nekkon,

I'm not part of the ng-openapi-gen team, I just ended up here as part of finding out that the bug in https://github.com/cyclosproject/ng-openapi-gen/issues/280 has finally been fixed. In practice all this change should do is add the previously missing Content-type header to JSON parts in multipart requests which was previously missing, so the reason for this change is this.

I wouldn't really say that this is a data transformation, it's just a change in how the byte stream of a request body (or body part) is represented when building the request, and since this in theory shouldn't affect anything other than the content type header of multipart JSON request body parts, can you please elaborate on what exactly this change has broken for you?

nekkon commented 10 months ago

Hey @karsa-mistmere

if (typeof value === 'object') {
      return JSON.stringify(value); // This was removed
      return new Blob([JSON.stringify(value)], {type: 'application/json'}) // Was replaced with this
 }

If you check the code change other than adding {type: 'application/json'} it also changes the string returned to a Blob. That conversion is the breaking change for us. The builder receives an object and sends a Blob to the server. If this change only added the missing Content-type header you mention then that would work for us fine too.

karsa-mistmere commented 10 months ago

If you check the code change other than adding {type: 'application/json'} it also changes the string to a Blob. That conversion is the breaking change for us. It is a data transformation on the output that is sent to the server. Before it was sending a string but now it sends a Blob.

I'm not quite sure what you mean by this, HTTP requests are binary and have no insight into internal Javascript types. A Blob object cannot practically be sent through HTTP as is, only its internal byte stream, and even though the stringified JSON is wrapped in a Blob, the request body should in practice be the same since the byte stream is the same. I've tested this and the requests themselves are the exact same apart from the added Content-type header.

Can you post your request method signature and what the requests end up being with and without the Blob wrapper?

nekkon commented 10 months ago

I'm not quite sure what you mean by this, HTTP requests are binary and have no insight into internal Javascript types.

Not sure where you read this or what you mean exactly. HTTP (Hypertext Transfer Protocol) requests are not binary in nature. HTTP is a text-based protocol, and its messages, including requests, are primarily composed of plain text. The structure of an HTTP request is defined by a set of headers and an optional body, and these components are typically represented using ASCII characters.

Here is an example of the impact of this change. In the screenshot below you can see how before this breaking change, along with a file in the multi-part form request's payload a stringified object is sent to the BE.

Screenshot 2024-01-11 at 11 34 04

Instead of the above, after this breaking change, the below is sent to the server.

Screenshot 2024-01-11 at 11 37 55

Data, although an object, is sent as binary in the request's payload.

karsa-mistmere commented 10 months ago

Happy to send you an example of the impact of this change.

Well, please do, I'm really curious how and why this could break anything, and I'm sure ng-openapi-gen's maintainers will also need all the information they can get.

karsa-mistmere commented 10 months ago

@nekkon: Can you post the requests in a standard format, e.g. cURL? (Removing any API keys et cetera if necessary).

Chrome saying "binary" doesn't really tell us what the request bodies look like.

E.g. for me this is the only difference between the two requests without and with the Blob wrapper. (Extra Content-type header for JSON part and a dummy filename value in Content-Disposition):

------WebKitFormBoundaryVsEO8P07CPBOEGqq
Content-Disposition: form-data; name="user"

{"foo":"bar"}
------WebKitFormBoundaryVsEO8P07CPBOEGqq
Content-Disposition: form-data; name="userImage"; filename="avatar.png"
Content-Type: image/png

------WebKitFormBoundaryVsEO8P07CPBOEGqq--
------WebKitFormBoundaryKP3DBsuHNBPRvn4W
Content-Disposition: form-data; name="user"; filename="blob"
Content-Type: application/json

{"foo":"bar"}
------WebKitFormBoundaryKP3DBsuHNBPRvn4W
Content-Disposition: form-data; name="userImage"; filename="avatar.png"
Content-Type: image/png

------WebKitFormBoundaryKP3DBsuHNBPRvn4W--
nekkon commented 10 months ago

Before the breaking change:

------WebKitFormBoundaryufI1j6YVweV7J9dO Content-Disposition: form-data; name="data" {"description":"test description","displayInFutureReports":false,"fileType":"cost_report","source":"test source","title":"test title"} ------WebKitFormBoundaryufI1j6YVweV7J9dO Content-Disposition: form-data; name="file"; filename="2721-Our-Behaviours---UK-English-Final.pdf" Content-Type: application/pdf ------WebKitFormBoundaryufI1j6YVweV7J9dO--

After the breaking change:

------WebKitFormBoundaryVEwxz7GQVxkJir6W Content-Disposition: form-data; name="data"; filename="blob" Content-Type: application/json {"description":"test description","displayInFutureReports":false,"fileType":"cost_report","source":"test source","title":"test title"} ------WebKitFormBoundaryVEwxz7GQVxkJir6W Content-Disposition: form-data; name="file"; filename="2721-Our-Behaviours---UK-English-Final.pdf" Content-Type: application/pdf ------WebKitFormBoundaryVEwxz7GQVxkJir6W--

The change shouldn't have added to the request filename="blob" as you noticed too. If the goal of the change was to only add the Content-Type then the result should have been:

Content-Disposition: form-data; name="data";
Content-Type: application/json
{"description":"test description","displayInFutureReports":false,"fileType":"cost_report","source":"test source","title":"test title"}

The filename attribute is used to suggest the name of a file being sent as part of the form data. Adding a redundant filename or wrapping up the JSON data in a blob doesn't seem correct to me as we are not sending a file but a stringified JSON. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition#filename

nekkon commented 10 months ago

Checked what needs to be done in order to create a pull request myself. It seems that this change is the only option in order to add the Content-Type. From what I read, you can not manually set the Content-Type of FormData when passing a string. Closing this as it seems there was no other way of achieving this.

harfyt commented 8 months ago

Checked what needs to be done in order to create a pull request myself. It seems that this change is the only option in order to add the Content-Type. From what I read, you can not manually set the Content-Type of FormData when passing a string. Closing this as it seems there was no other way of achieving this.

Hi @nekkon, How did you manage to overcome this change? I am facing the exact problem that you described. I am using a "data" FormData entry as a json stringified object and send it with a file and then at Server side I am deserializing the json string to the desired object type. With the above change I am not having the "data" entry any more in my FormCollection but instead i am getting a second file with name "blob".