Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: can't pass null to a setter with validation #28

Merged
merged 2 commits into from
Feb 14, 2021
Merged

Conversation

jods4
Copy link
Contributor

@jods4 jods4 commented Feb 14, 2021

Fixes #22

I first looked if this was better fixed in the core lib or generated code.
The SetXxxWithValidation methods don't have enough context to know if null is an acceptable value. They only check against the target xsd simple type. Whether null is ok or not depends on the target element/attribute being optional in context.
So I fixed this at the codegen level, which conveniently also works around the value.ToString() NRE for enum properties.

Codegen comparison

Using wss.xsd (from the tests) as a baseline, here's the code change for a nullable enum:

public virtual System.Nullable<EventReceiverType> Type {
            get { /*... */ }
            set {
+                if (value == null) {
+                    this.SetElement(TypeXName, null, XmlSchemaType.GetBuiltInSimpleType(XmlTypeCode.String).Datatype);
+                }
+                else {
                    this.SetElementWithValidation(TypeXName, value.ToString(), "Type", global::Microsoft.Schemas.SharePoint.EventReceiverTypeValidator.TypeDefinition);
+                }
            }
        }

It also applies to references, from the same wss.xsd:

        public virtual string SolutionId {
            get { /*...*/ }
            set {
+                if (value == null) {
+                    this.SetElement(SolutionIdXName, null, XmlSchemaType.GetBuiltInSimpleType(XmlTypeCode.String).Datatype);
+                }
+                else {
                    this.SetElementWithValidation(SolutionIdXName, value, "SolutionId", global::Microsoft.Schemas.SharePoint.UniqueIdentifier.TypeDefinition);
+                }
            }
        }

Additional changes

I slipped in two additional changes that may not be obvious:

  1. I now call value.ToString() when setting an enum on an element plain value (text content). It made the new code easier to write and I just can't see why it wasn't done in this case?
    See this comment for additional details:
    Impossible to set nullable properties to null #22 (comment)

  2. On the codepath that doesn't perform validation, I realised that value.ToString() can also NRE if value is a nullable enum.
    You could say: "but if value is an enum, there would always be validation, so that can't happen!". In theory yes, except I noticed that validation is explicitely disabled for attributes, see this comment for details:
    Impossible to set nullable properties to null #22 (comment)
    I have no idea why it's so and didn't change it.
    I transformed the codegen to use value?.ToString() though, so that it wouldn't crash with a nullable enum.
    This syntax means users need at least C# 6 to compile the new code, because of the ?. operator.
    C# 6 is old today, esp. for users targetting .NET Core, so I think that's ok.

Example from wss.xsd:

        public virtual System.Nullable<Microsoft.Schemas.SharePoint.ReferenceType> Type {
            get { /* ... */ }
            set {
-                this.SetAttribute(TypeXName, value.ToString(), XmlSchemaType.GetBuiltInSimpleType(XmlTypeCode.String).Datatype);
+                this.SetAttribute(TypeXName, value?.ToString(), XmlSchemaType.GetBuiltInSimpleType(XmlTypeCode.String).Datatype);
            }
        }

Implementation notes

You'll notice I made a liberal usage of CodeSnippetExpression.
This uses code in plain strings and has a few benefits:

  • It makes the code easier to read and write as I think value == null is much easier on the eyes than the equivalent CodeDom.
  • It's more performant as it allocates less and that string is printed verbatim in the output.

It could have a few drawbacks but I don't think they apply here:

  • It wouldn't work if we wanted to generate VB sources.
  • It is not analyzed by the code that looks for identifiers in scope (to find unique names), but I never declare new identifiers this way.

Also notice that I haven't re-generated the sources in the test project, as I'm not sure what the process is.

This was referenced Feb 14, 2021
@mamift mamift merged commit 2a28c7b into mamift:master Feb 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impossible to set nullable properties to null
2 participants