Address Protocol Tests that are breaking changes #3474
Conversation
| { | ||
| "core": { | ||
| "changeLogMessages": [ | ||
| "fix: address protocol tests that are breaking changes." |
There was a problem hiding this comment.
I would prefer this is a little more specific to what is breaking since this is the change log.
| if (!withPrefix) | ||
| { | ||
| var locationNameString = locationName.ToString(); | ||
| return locationNameString.StartsWith("xsi:") ? locationNameString.Substring(4) : locationNameString; |
There was a problem hiding this comment.
Is the prefix always going to be xsi or should we be doing a more generic prefix strip?
| @@ -123,7 +123,9 @@ namespace <#=this.Config.Namespace #>.Model.Internal.MarshallTransformations | |||
| { | |||
| #> | |||
| if(<#=variableName + ".IsSet" + member.PropertyName#>()) | |||
| xmlWriter.WriteAttributeString("<#=member.MarshallName#>", "<#=member.XmlNamespace#>",<#=member.PrimitiveMarshaller#>(<#=variableName#>.<#=member.PropertyName#>)); | |||
| <#+ | |||
| WriteXmlAttributeString(level + 1,member,variableName, isPayload: false); | |||
There was a problem hiding this comment.
readability: missing spaces after the ,s
| var prefix = ""; | ||
| var localName = ""; | ||
| string ns = member.XmlNamespace; | ||
| if (member.MarshallName.StartsWith("xsi")) |
There was a problem hiding this comment.
Should this be generic prefix parsing instead?
There was a problem hiding this comment.
Just for some context
Normally a service would model it like this where they tell us the prefix. In that case we use the prefix provided
But in some cases the service will actually attach xsi to the locationName itself like this
Although no actual service does this, the protocol tests want to make sure we can handle that. The only other valid prefix from what I've seen is xsd (other than xsi), but I don't see a problem with just assuming everything before the first : is a prefix, since we can't unmarshall an attribute with a : in it anyways. Will make it more generic.
peterrsongg
force-pushed
the
petesong/address-v4-protocol-tests
branch
from
57bce14
to
80b7df3
Compare
| @@ -64,7 +64,7 @@ public IRequest Marshall(XmlAttributesRequest publicRequest) | |||
| { | |||
| xmlWriter.WriteStartElement("XmlAttributesRequest", ""); | |||
| if(publicRequest.IsSetAttr()) | |||
| xmlWriter.WriteAttributeString("test", "",StringUtils.FromString(publicRequest.Attr)); | |||
| xmlWriter.WriteAttributeString("","test", "",StringUtils.FromString(publicRequest.Attr)); | |||
There was a problem hiding this comment.
Is the extra space before xmlWriter intentional? If you're making a change, it'd also be good to add spaces between the ,, e.g.:
xmlWriter.WriteAttributeString("", "test", "", StringUtils.FromString(publicRequest.Attr))There was a problem hiding this comment.
ah good catch, removed the extra indentation
| #> | ||
| <#+ | ||
| // xsi is a common prefix in attributes, but it cannot be included in the local name itself as the xmlWriter class will throw an exception. | ||
| // Some services may model the xmlName, with the xsi prefix included like it is here https://github.com/smithy-lang/smithy/blob/main/smithy-aws-protocol-tests/model/restXmlWithNamespace/main.smithy#L147 |
There was a problem hiding this comment.
Small nit: In the GitHub UI, you can right click that line and select Copy permalink.
I don't know how often those protocol tests change, but if they do line 147 on the main branch may not be the same.
There was a problem hiding this comment.
cool tip! didn't know about that. just updated
Description
This addresses all the Protocol Tests which we deemed as breaking changes
SimpleScalarPropertiesPureWhiteSpace: Xml responses that returned pure whitespace in the body of an xml element were being trimmed. For exampleRestJsonHttpPayloadTraitsWithNoBlobBodyHttpPayloadTraitsWithNoBlobBody: If the payload of a response has no data, instead of returning an empty memory stream as the payload we will return null.@xsi:was not working, since@xsiis a prefix. We will now successfully unmarshall an attribute with a prefix.Some refactoring of the tt files.
Some other tests were request level changes that weren't breaking but were fixed with the nullability changes (see empty lists/maps tests)
Note The other tests in the VNext list can't be addressed until we pull in the latest smithy version, since I added some additional tests for nestedXmlMapsWithXmlName. This would also cause us to pull in a lot of other protocol tests, so should be addressed in a separate PR. Those tests are not breaking changes so it is not critical to ship them in V4.
Motivation and Context
Need to address in major version as there are breaking changes.
DOTNET_7435
Testing
DRY_RUN passed in v4.
All protocol tests passed.
Screenshots (if appropriate)
Types of changes
Checklist
License