Message properties that are optional need to be nil-able #93
Conversation
|
@serbrech, adding you here in case you have any feedback about this. This would definitely be a breaking API change but we've had a few of those. From my reading (and talking with @jhendrixMSFT I believe he agrees) properties that are of string type but optional need to allow for a "not specified at all" or "empty string" distinction that we can't represent today in our message. |
|
Per http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-types-v1.0-os.html#doc-idp115568, all non-mandatory fields can have the null value. Therefore, we should make all non-mandatory fields pointer-to-type. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
pretty confident that we have never used any of these properties, and it's the right thing to do anyways. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
(got one test failing, not sure if it's related to my change. Changing the pipeline YAML to output the actual test output) |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I had a flaky test failure with the multi-part message (seems unrelated to my change for fields since those are basically "field is nil or not". To diagnose this I added a step to dump the output to stdout, and now the tests are passing consistently. |
|
Also, could you check to see if other types need this same fix-up? You can roll them into this PR or separate ones. |
…erties in here at all as per the spec, so each one should be nilable. I also added in some type aliases to make it simpler to match the spec to the individual types, at least for message properties.
Okay, I've done another update (still just focusing on message properties, but those are the commonly used ones). This time I created type aliases so the name of the type matches more with the actual type in AMQP. They're all pretty simple. I also made every optional property actually be optional by making them pointers. @jhendrixMSFT , can you take another look? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Also, filed #114 to look at any other properties to see which ones could use the aliased types. |
|
Pushed another change to make sure that the marshaller gets exercised with nil properties (also a fix) |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Some fields that are optional in AMQP were typed as just plain 'string', which meant you couldn't pass an empty string.
This PR changes those fields to now be pointers to string instead. This is an absolutely breaking change for any callers, and could use a bit more testing (I just ran it through the current tests).