Change OpenApiByte & OpenApiBinary #391
Conversation
| return new OpenApiBinary(Encoding.UTF8.GetBytes(value)); | ||
| } | ||
| catch(Exception) | ||
| { } |
There was a problem hiding this comment.
Try/catch unnecessary. #Closed
There was a problem hiding this comment.
Leave the uplevel to process the exception? #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
GetBytes(..) may throw the following exception:
Exceptions:
// T:System.ArgumentNullException:
// s is null.
//
// T:System.Text.EncoderFallbackException:
// A fallback occurred (see Character Encoding in the .NET Framework for complete
// explanation)-and-System.Text.Encoding.EncoderFallback is set to System.Text.EncoderExceptionFallback. #Closed
There was a problem hiding this comment.
Got it. Let's do this:
- The ArgNullEx one won't be thrown since we already handle it above.
- Can we specifically catch the second type of exception?
In reply to: 265771477 [](ancestors = 265771477)
| : this(new byte[] { value }) | ||
| { | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove this #Closed
There was a problem hiding this comment.
Remove this is a breaking change. We should avoid breaking change.
#Closed
There was a problem hiding this comment.
Yes, but this is technically a bug fix since our old model is incorrect
In reply to: 265768029 [](ancestors = 265768029)
There was a problem hiding this comment.
public OpenApiByte(byte value) is not a technically bug. Keeping it also provides a method for customer to use. #Closed
There was a problem hiding this comment.
| @@ -119,7 +120,8 @@ public void SerializeAdvancedExampleAsV3JsonWorks() | |||
| { | |||
| ""href"": ""http://example.com/1"", | |||
| ""rel"": ""sampleRel1"", | |||
| ""binary"": ""AQID"" | |||
| ""bytes"": ""AQID"", | |||
| ""binary"": "")*+"" | |||
There was a problem hiding this comment.
)*+ [](start = 26, length = 3)
For the binary case, can we test with a bit more complicated string (something that's not in ASCII)?
Instead of putting in the byte[] specifically above for the binary case, you can use the UTF8.GetBytes function to get the right byte array. #Closed
There was a problem hiding this comment.
I original use the byte[] { 1, 2, 3 }, but, the output string is not displayable. How can I show that? #Closed
There was a problem hiding this comment.
Yeah, that's because 1, 2, 3 correspond to some control characters in ASCII (thus non-printable).
I was suggesting something along this line:
- Here in line 124, do something like ""Ñ😻😑♮Í☛oƞ😲☇éNjŁ😟¥a´Ī♃ƠƩ""
- Then, in line 35, do UTF8.GetBytes("Ñ😻😑♮Í☛oƞ😲☇éNjŁ😟¥a´Ī♃ƠƩ")
In reply to: 265777035 [](ancestors = 265777035)
There was a problem hiding this comment.
:) I changed. #Closed
|
I will sign off, but given that this issue was quite contentious (for lack of a better word), could we get all 3 of us to agree before checking this in? That is, @darrelmiller, could we get your comments and/or sign-off here as well? |
|
Thanks @xuzhg for fixing this and thanks @PerthCharern for catching the issue after I had signed off on it! |
|
Thanks @darrelmiller! I'll merge this in. Thanks @xuzhg for detecting and addressing the issue. |
Below are the discussions:
@darrelmiller
Admittedly, the schema is unnecessary, but there are examples in the spec that show this. In this scenario the format: binary is used to indicate that there is no special serialization that is performed on the bytes. They are raw. If you try and present them as a string, they might not create printable text.
This is not a problem in 90% of cases because for the definition of an API you don’t need to include any data. The spec says the parameter contains binary data and tooling knows not to do any processing on it. That’s obviously potentially tricky if it is in a URL parameter! However, for us, the only place it causes a problem is in examples. In examples it is important that the user chooses example data that can be serialized into printable strings. Or not include a sample for binary data as it is somewhat redundant.
This brings me back to my suggestion that for examples of binary data, if they are present, we should just assume that they can successfully be rendered as UTF8 strings and that they will roundtrip. If it doesn’t work, then the user should choose an example of binary data that can be stored in UTF-8 because that’s a limitation of the OpenAPI spec.
This will ensure that round tripping works just fine. However, if someone manually creates an OpenAPI document via the DOM and assigns an arbitrary set of bytes that cannot be represented as UTF-8 characters then it will not round trip. Ironically, this is good because it will discourage people from using format “binary”. If we “fix” this bug to make it serialize properly then we will hide the problem and people will expect other OpenAPI implementations to process the issue properly. But we don’t control how other implementations will interpret binary.