Event Serialization code should handle NULL strings correctly

[mrjerryjohns]

For WDM events, the expectation on applications that emit them is to memset them to 0 before using them. This protects the application against additions made to those events being made in schema before they may have had a chance to update their code (or they may not even care to set that field).

For fields of type string, the serialization utils in Weave do not correctly handle a NULL pointer and blit out an empty string for that field in the payload. This has caused crashes in application code.

In keeping with the theme that 'memset' is the right way to initialize an event, then this should follow suit as well.

[jaylogue]

Your explanation doesn't make it clear what the correct behavior should be. Are you saying that the code should blit out an empty string in the payload for NULL field?

Why an empty string, instead of an absent field? What about cases where an empty string violates the schema?

If a string field is added to an event, but the application hasn't been updated to set it, then that field must have been marked optional in the schema, and therefore should simply be elided from the encoding.

[mrjerryjohns]

The goal of initialization here is to setup safe, clear defaults in the constructed data. Similar to how the default would be 0 for integers, false for booleans, empty for arrays, we need a 'default' for strings such that it would be a NULL-string by default (i.e "").

Why an empty string, instead of an absent field? What about cases where an empty string violates the schema?

That is not for Weave to enforce - by that analogy, we should be setting up every non-optional field as part of 'initialization' to be 'unset', and force the application to set them explicitly. Clearly we're not doing that for some WDL types like integers, booleans, so picking strings to make this enforcement happen is not consistent.

Certainly having the code crash as a way to discover that isn't right. Having a mode where a validator runs to ensure that the application has set every non-optional field might be a better way to catch this (a feature only available in debug builds perhaps).

If a string field is added to an event, but the application hasn't been updated to set it, then that field must have been marked optional in the schema, and therefore should simply be elided from the encoding.

This likely applies more broadly than just strings, (optional bools, integers too). In that case, would it be that all optional fields are by default on initialization, 'not set'? Perhaps an issue for another day...

[jaylogue]

Certainly having the code crash as a way to discover that isn't right.

Couldn't it just return an error?

I can live with sending an empty string here. But I certainly don't agree that silently sending the wrong thing is somehow better than overtly crashing (with a clear explanation why) when the programmer forgets to initialize a field.