Bug 350201

Comment 1 Jarek Gawor 2011-06-23 17:38:52 EDT

Created attachment 198502 [details]
Patch for this issue.

The attached patch 1) checks if the type attribute is null, 2) provides a better error message is wrong type attribute is specified, and 3) checks for null in MetaTypeProviderImpl.readMetaFiles() when parser.doParse() fails.

Comment 2 John Ross 2011-06-30 15:47:14 EDT

Created attachment 198932 [details]
updated patch

This patch updates the previous one as follows.

I removed all of the exception catching and print stacks from DataParser.doParse and just let the method throw an exception.

That exception is caught and logged in MetaTypeProviderImpl.readMetaFiles. This also resulted in removing some unnecessary throws declarations in method signatures.

I beefed up the exception reporting in certain places by including the metadata XML URL and bundle ID as part of the log message.

Updated copyrights where necessary.

Bumped up the bundle version from 1.1.0 to 1.1.100.

Comment 3 John Ross 2011-06-30 15:53:52 EDT

I'm wondering if the log output in MetatypeProviderImpl.readMetaFiles should be WARNING instead of ERROR since parsing will not stop. All metadata files that can be parsed will be parsed.

Comment 4 John Ross 2011-06-30 16:44:39 EDT

Actually, I'm not sure I like continuing if there's an issue with any of the metadata XML files in a bundle. Seems simpler to just fail the whole thing. I also don't think a bundle with bad metadata XML should be considered the same as a bundle with no metadata XML (which results in a MS/MSF/MetaTypeProvider service tracker). The problem is the MetaTypeService.getMetaTypeInformation method has no declared exceptions. I wonder if we should just log an error and throw a runtime exception.

Comment 5 Thomas Watson 2011-06-30 16:54:31 EDT

(In reply to comment #4)
> Actually, I'm not sure I like continuing if there's an issue with any of the
> metadata XML files in a bundle. Seems simpler to just fail the whole thing. I
> also don't think a bundle with bad metadata XML should be considered the same
> as a bundle with no metadata XML (which results in a MS/MSF/MetaTypeProvider
> service tracker). The problem is the MetaTypeService.getMetaTypeInformation
> method has no declared exceptions. I wonder if we should just log an error and
> throw a runtime exception.

The problem with throwing a runtime exception is that no client will be prepared to react to it.  This likely will result in breaking the front end (UI, webconsole etc.) to the metatype service and then result in more bugs getting opened against the Equinox metatype implementation.

If an error occurs I say simply log it and continue.  We could decide that the service tracker should not get created when invalid xml data is parsed, but I am no too concerned here.  If a bundle has invalid XML data AND is registering their own MetaTypeProvider services then they are doubly wrong.  I don't really care what we do for them, but would like to avoid throwing runtime exceptions to clients of the metatype service impl simply because that is likely to completely bust the clients front end.

Comment 6 John Ross 2011-06-30 17:09:15 EDT

(In reply to comment #5)
> The problem with throwing a runtime exception is that no client will be
> prepared to react to it.  This likely will result in breaking the front end
> (UI, webconsole etc.) to the metatype service and then result in more bugs
> getting opened against the Equinox metatype implementation.
> If an error occurs I say simply log it and continue.  We could decide that the
> service tracker should not get created when invalid xml data is parsed, but I
> am no too concerned here.  If a bundle has invalid XML data AND is registering
> their own MetaTypeProvider services then they are doubly wrong.  I don't really
> care what we do for them, but would like to avoid throwing runtime exceptions
> to clients of the metatype service impl simply because that is likely to
> completely bust the clients front end.

Current behavior is an attempt is made to parse all metadata files provided by a bundle. If any fail to parse an error is logged and parsing continues. If all metadata files fail to parse, the bundle is seen as having no metadata XML and converted into a tracker. There is also a chance getMetaTypeInformation(Bundle) returns null if an unexpected exception occurs.

Comment 7 John Ross 2011-06-30 18:50:48 EDT

Created attachment 198940 [details]
updated patch

I think the current behavior is okay. It seems pretty clear the spec does not want you to throw an exception or return null for MetaTypeService.getMetaTypeInformation(Bundle). To that end, this patch cleans up the possibility of returning null. Null will never be returned and only an Error will ever be thrown. If all else fails, a MetaTypeInformation with an underlying tracker is returned. Note that the tracker is a singleton.

This patch also includes the fix for bug 346804.

Comment 8 John Ross 2011-07-01 15:28:24 EDT

Patch released.

Comment 9 John Ross 2011-07-01 15:28:57 EDT

Comment on attachment 198502 [details]
Patch for this issue.

Thanks for the patch.