Add null checks in various XML methods #73243
Conversation
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/area-system-xml |
| @@ -58,16 +58,19 @@ public class XmlSchema : XmlSchemaObject | |||
|
|
|||
| public static XmlSchema? Read(TextReader reader, ValidationEventHandler? validationEventHandler) | |||
| { | |||
| ArgumentNullException.ThrowIfNull(reader); | |||
There was a problem hiding this comment.
What was the behavior before the check was added? NullReferenceException?
There was a problem hiding this comment.
NullRef for almost all, I can't remember which one but one of them also failed in some other unexpected way
There was a problem hiding this comment.
"Root element is missing." for XPathDocument..ctor(TextReader) when TextReader is null. That's just purely incorrect (we passed it into some other methods without validation until one of them eventually failed with random exception)
There was a problem hiding this comment.
seems it was also XmlSchema.Write and there is testcase which is testing that which failed (I missed it on the CI because of other unrelated errors). I'll go ahead and change that behavior since this error doesn't seem particularly useful:
Unhandled exception. System.InvalidOperationException: There was an error generating the XML document.
---> System.NullReferenceException: Object reference not set to an instance of an object.
at System.Xml.Serialization.XmlSerializationWriter.WriteStartDocument()
at Microsoft.Xml.Serialization.GeneratedAssembly.XmlSerializationWriterXmlSchema.Write64_schema(Object o)
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
--- End of inner exception stack trace ---
at System.Xml.Serialization.XmlSerializer.Serialize(XmlWriter xmlWriter, Object o, XmlSerializerNamespaces namespaces, String encodingStyle, String id)
at System.Xml.Schema.XmlSchema.Write(XmlWriter writer, XmlNamespaceManager namespaceManager)
at XmlSchemaWriteRepro.Program.Main(String[] args) in C:\Users\kwicher\source\repos\XmlSchemaWriteRepro\XmlSchemaWriteRepro\Program.cs:line 13
| [Fact] | ||
| public static void XPathDocumentNullStream() | ||
| { | ||
| Assert.Throws<ArgumentNullException>(() => new XPathDocument(default(Stream))); |
There was a problem hiding this comment.
there is also TextReader overload and we want to make sure it picks the right one
There was a problem hiding this comment.
Apples vs oranges. No strong preference either way. More important to test behavior rather than if we do it one way or another
|
no changes, just rebase |
|
Seems there was a test failure where we for some reason expected InvalidOperationException for null argument (with completely useless error message and NullRef on the inner exception), see here for more details: #73243 (comment) - I went ahead and removed this (since I already added test asserting ArgumentNullException) - I don't expect any particular impact from this change since it only happens for null arguments which suggests design error rather than something which app could potentially catch - makes more sense to throw more useful error message. Also now we match the behavior written in docs |
|
Any of these worth a breaking change note? (I'm guessing not) |
|
I don't think so, we're now matching behavior to already documented behavior and this exception seems very unlikely to be found by random input - most likely will be only seen during app-design time by developer. |
Fixes #41061