Improve null-safety in Factories and cleanup

[ArnyminerZ]

Code cleanup, including new utility functions and a new XmlReader class which simplifies the factory classes.

[rfc2822]

We should have an approach that isn't specific to PushTopic or another specific element.

Most often the exception is logged because requireReadText() is used, which throws an exception when a property doesn't have a text value, although that can be perfectly valid:

# (taken from RFC 4918 p. 37)
…
<D:propstat>
  <D:prop><R:DingALing/><R:Random/></D:prop>
  <D:status>HTTP/1.1 403 Forbidden</D:status>
</D:propstat>
…

Here we would have the R:DingALing and R:Random elements which could not be parsed when they don't have content, but use requireReadText(), for instance because they usually look like <R:Random>Some Text</R:Random>.

I think it's better to not make any assumptions about the content of elements in dav4jvm (maybe except if explicitly required by the standard, but even then we should parse as forgiving as possible). If at all, this should be left to the application that uses dav4jvm.

So, I suggest to

• drop requireReadText() because it does things that shouldn't be done (throw exception when contents are unexpected),
• use readText() everywhere instead,
• expect all values to be null (like in AddressbookDescription).

So for instance the Push Topic should also have a val topic: String? that can be null.

[ArnyminerZ]

I've deprecated requireReadText instead of removing it, but we can simply get rid of it and call it a day, I don't know how many people is using this library and that utility specifically.

[rfc2822]

I think it would solve the existing problem (see #9) that this can't be parsed without information loss:

<multistatus>
… with 404: …
<prop>
  <getetag/>
</prop>
</multistatus> 

to consequently allow empty properties.

So in the case of GetETag, we have to deal with the situation above, so it should be possible to generate an empty GetETag. Who knows, maybe the receiving app needs to list the 404 properties, or maybe there's at some day something like:

<this-server-supports>
  <getetag/>
</this-server-supports>

Then we also need the empty GetETag element.

So, we should parse and return a GetETag in this case, but without content (String value may be null).

Same applies to all elements.

[rfc2822]

We should drop it completely.

[ArnyminerZ]

Same applies to all elements.

Do you mean all? Like, make PropertyFactory.create return non-null? Or just Topic, GetETag and PushSubscribe?

[ArnyminerZ]

For example CalendarColor requires an int color, we can always set to black or something like that but I'm not sure if we want that

[rfc2822]

I'd allow null too for the same reason: It may be interesting for an application to know that an empty element was transmitted.

I have thought about returning some kind of for instance Empty<CalendarColor> when CalendarColor couldn't be parsed with value, but it makes thing unnecessary complex.

The HrefListProperty properties should automatically work. The others often already allow null values. I think we should consequently allow null values in all properties, also to be consistent.

[rfc2822]

Real-world example of misleading logs from accessing a WebDAV folder in DAVx5:

		<d:propstat>
			<d:prop>
				<d:current-user-privilege-set/>
				<d:getcontenttype/>
				<d:getcontentlength/>
			</d:prop>
			<d:status>HTTP/1.1 404 Not Found</d:status>

because the folder doesn't have a getcontentlength property causes in the logs:

15:06:09.682  8584-8609  at.bitfire.dav4jvm.Property     D  Ignoring unknown property DAV::getcontentlength

although the property is known (but it can be parsed because it currently requires a value).

[ArnyminerZ]

Okay, I'll make all properties nullable then 😉

[rfc2822]

Some requests 😃

Also please provide a short PR summary. It can be very short like "Currently there's misleading logging, so now we do this and that: - …, - …", but at least to have an idea what it's about.

And it's often not exactly the same the issue, like in this case. The issue is just about the logging, but the PR is now about null values etc.

[ArnyminerZ]

Should be ready. Sorry for the long PR, but there are a lot of classes that use the same methods 😅

[rfc2822]

Looks good, logging should be more intuitive now and we've on the go also fixed the root cause of #9.