Open MartinM85 opened 3 months ago
Thanks for raising this @MartinM85
Any chance you can confirm that the seriliazed payload is as expected when you run the following?
var stringValue = KiotaSerializer.SerializeAsString("multipart/form-data", multipartBody);
Hi @andrueastman, Text Visualizer in VS shows me this
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
string value:
"--605a817187c144bbae08f23dda69523a\r\nContent-Type: application/x-www-form-urlencoded\r\nContent-Disposition: form-data; name=\"username\"\r\n\r\ntestn\r\n\r\n--605a817187c144bbae08f23dda69523a\r\nContent-Type: application/x-www-form-urlencoded\r\nContent-Disposition: form-data; name=\"password\"\r\n\r\nabc\r\n\r\n--605a817187c144bbae08f23dda69523a--\r\n"
In MultipartBody.Serialize
, I see calling of AddNewLine(writer)
;
It looks like since the value is a string value, the writer isn't using the serializer based on the content type the same way it would for a parsable but using the multipart writer instead hence the new line character.
We would need to update the if clause to be similar to the parasable clause above it to pull the right serializationWriter
.
Any chance this is this something you would be willing to submit a PR for @MartinM85?
Hi @andrueastman I will try to deliver a fix
@andrueastman Fix delivered, but I'm worried about unit tests. Unit tests for MultipartBody are part of Abstraction library, but in this case, we need to set up concrete serialization writer. Not sure if mocking of json serializer is a good idea. Seems like only https://github.com/microsoft/kiota-abstractions-dotnet/blob/0787139294b160818a89b22d24eb079084fc7ffd/src/MultipartBody.cs#L149 is covered by the current unit tests.
It would be nice to have some integration tests to ensure that everything will work.
What I'd suggest for now is adding a test and referencing the project in the multipart library. Similar to this. https://github.com/microsoft/kiota-serialization-multipart-dotnet/blob/e0b280eb3304d009592e79cf074a2867d984c604/Microsoft.Kiota.Serialization.Multipart.Tests/MultipartSerializationWriterTests.cs#L98. The build will fail for now but pass once released and capture this going forward.
This should all get easier to do later on once we move the projects to one repo to make the end to end testing easier for such via https://github.com/microsoft/kiota-abstractions-dotnet/issues/238
@andrueastman Seems like my change didn't work, but it's quite challenge to test it properly. I've tried to add unit test to https://github.com/microsoft/kiota-serialization-multipart-dotnet, but I need to reference local Microsoft.Kiota.Abstractions and Microsoft.Kiota.Serialization.Json. It's time consuming and what's important, I don't know what should be the expected result.
Based on this: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition#examples the correct serialized content should be:
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
Maybe the fix should be removing AddNewLine: https://github.com/microsoft/kiota-abstractions-dotnet/blob/0787139294b160818a89b22d24eb079084fc7ffd/src/MultipartBody.cs#L179, but removing this line can break another funcionality.
@MartinM85 FYI Andrew is out, we're not sure when he'll be back at this point. What result are you getting today?
@baywet In the controller, the properties values are null
let me be more specific with my question: when you send the payload from the client today, how it is formatted? and how does it differ from what would be required?
Behavior for version 1.9.3:
var client = new ApiClient();
var multipartBody = new MultipartBody();
multipartBody.AddOrReplacePart("username", "application/x-www-form-urlencoded", "test");
multipartBody.AddOrReplacePart("password", "application/x-www-form-urlencoded", "abc");
var tokenResponse = await client.Api.Auth.Login.PostAsync(multipartBody);
Serialized payload when calling:
var stringValue = KiotaSerializer.SerializeAsString("multipart/form-data", multipartBody);
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
Server:
Login.Username has value "test\r\n"
Login.Password has value "abc\r\n"
Behavior for version >=1.9.4:
var stringValue = await KiotaSerializer.SerializeAsStringAsync("multipart/form-data", multipartBody);
throws System.InvalidOperationException: 'SerializationWriterFactory'
StackTrace:
at Microsoft.Kiota.Abstractions.MultipartBody.Serialize(ISerializationWriter writer) at Microsoft.Kiota.Serialization.Multipart.MultipartSerializationWriter.WriteObjectValue[T](String key, T value, IParsable[] additionalValuesToMerge) at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.SerializeAsStream[T](String contentType, T value) at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.SerializeAsStringAsync[T](String contentType, T value, CancellationToken cancellationToken)
Server:
Login.Username has value null
Login.Password has value null
Re-opening this one for now....
The error occurs as the property multipartBody.RequestAdapter
is not set (this is done automagically by the request adapter when sending the request.) So you will need to add a line like multipartBody.RequestAdapter = graphClient.RequestAdapter;
before calling the serializer.
It also looks like the writer for x-www-form-urlencoded
will not write values if a key is not specified as the multipart writer will pass the use the key as the part name. See https://github.com/microsoft/kiota-serialization-form-dotnet/blob/50d7a29ac768c2f3c0047d8909f12c2df623a30f/src/FormSerializationWriter.cs#L203
Using the code below will yield the expected result albeit with the wrong content type(i.e. "text/plain").
var multipartBody = new MultipartBody();
multipartBody.AddOrReplacePart("username", "text/plain", "test");
multipartBody.AddOrReplacePart("password", "text/plain", "abc");
multipartBody.RequestAdapter = graphClient.RequestAdapter;
var stringValue = await KiotaSerializer.SerializeAsStringAsync("multipart/form-data", multipartBody);
Open question here is shouldn't the body for url-form-encoded be with key value pairs as below?
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
username=test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
password=abc
--605a817187c144bbae08f23dda69523a--
Trying the requests from Postman. API is running on ASPNET Core 8 1.
POST {app_url}/login
Content-Type: multipart/form-data;boundary=605a817187c144bbae08f23dda69523a
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="username"
username=test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="password"
password=abc
--605a817187c144bbae08f23dda69523a--
Username
is set to username=test
Password
is set to password=abc
2.
POST {app_url}/login
Content-Type: multipart/form-data;boundary=605a817187c144bbae08f23dda69523a
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
Username
is set to test
Password
is set to abc
3.
POST {app_url}/login
Content-Type: multipart/form-data;boundary=605a817187c144bbae08f23dda69523a
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
Username
is set to test\r\n
Password
is set to abc\r\n
looking at both RFC1867 and RFC7578 it is clear there shouldn't be a line return between the part and the next boundary (see different between previous message 2 and 3 examples). This is something we should fix.
Then I think there's a misunderstanding there. I believe that since the multipart format is already handling the "segmentation of multiple properties" is doesn't need to be repeated in the different parts. I.E test
not username=test
. And since the application is not doing a sub-segmentation (i.e. username is a string, not an object with properties), form encoded or text plain are functionally equivalent. I'd still recommend using text plain to avoid getting strange encoding behaviours with special characters (likely to happen for passwords).
Does that make sense?
looking at both RFC1867 and RFC7578 it is clear there shouldn't be a line return between the part and the next boundary (see different between previous message 2 and 3 examples). This is something we should fix.
Thanks @baywet, I believe this is already fixed in the latest abstractions with @MartinM85 PR.
I was just highlighting that using the code below
var multipartBody = new MultipartBody();
multipartBody.AddOrReplacePart("username", "text/plain", "test");
multipartBody.AddOrReplacePart("password", "text/plain", "abc");
multipartBody.RequestAdapter = graphClient.RequestAdapter;
var stringValue = await KiotaSerializer.SerializeAsStringAsync("multipart/form-data", multipartBody);
will result in the correct and expected result as below (no spaces but content type as text plain)
--2d3a3832fb72430abed317363391ca7d
Content-Type: text/plain
Content-Disposition: form-data; name="username"
test
--2d3a3832fb72430abed317363391ca7d
Content-Type: text/plain
Content-Disposition: form-data; name="password"
abc
--2d3a3832fb72430abed317363391ca7d--
However, changing the content type of the parts to application/x-www-form-urlencoded
results in empty values as below. (Hence the null values observed by @MartinM85.)
--45b491730a624687a4559322c1491f72
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
--45b491730a624687a4559322c1491f72
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
--45b491730a624687a4559322c1491f72--
This is because the form writer does not accept writing values when the key is not passed/present/empty at https://github.com/microsoft/kiota-serialization-form-dotnet/blob/50d7a29ac768c2f3c0047d8909f12c2df623a30f/src/FormSerializationWriter.cs#L203.
I believe the question really is that should we update the form writer to accept empty keys? Or does setting the content type as application/x-www-form-urlencoded
result in the expectation that key value pairs are expected?
In my head, the only valid case for using application/x-www-form-urlencoded
would be for an object where the partname would be object name and then we would write key value pairs for its properties in the payload.
So, a scenario like this should probably not have the part content type as application/x-www-form-urlencoded
since we only want to write the values. With this clarified we can probably close this issue now...
Before we make that last change in the form serialization, we should double check in the specs (I believe that form encoded is a split between RFC and WHATWG) if this is valid. Would you look into that @andrueastman ?
No worries. Will take a look into this to confirm.
Way forward will either be
Sure, thanks for keeping the issues clean. Please go ahead.
I've a controller
Login
class has propertiesUsername
andPassword
Part of swagger.json from which a client is generated
Calling the
login
method from the clientWhen debugging the
login
method, I see that the value inUsername
is"test\r\n"
and inPassword
is"abc\r\n"
. At the first sight it seems to me that CRLF are added during serialization.Same for content type
text/plain