Bug 355187

Comment 1 Robert Kiss 2011-08-22 09:19:06 EDT

Created attachment 201920 [details]
Patch file

As I'm not sure if I didn't broke anything, I prefer to submit a patch instead of committing the code.
The ideea of the change is very simple actually:
1: put the option CONTINUE_AFTER_FATAL_ERROR_FEATURE to true
2: when a loading of a file fail (eg: EcorePlatformUtil.loadModelRoot(editingDomain, file); return a null resource) mark that file as "damaged" by using a dedicated content type ID to avoid additional loading retries

Currently there is a test that fail because of my change, ModelSyncingTest.testAlreadyLoadedFileChanged but looking at the test I think is normal to fail and the test need to be reconsidered.

Stephan, can you please take a look and tell me your opinion about this?

Comment 2 Stephan Eberle 2011-09-27 11:37:53 EDT

I've reviewed and integrated the patch into the codebase. The commited code has the following changes wrt the submitted patch:
* Removed adding of exceptions caught by EcoreResourceUtil#loadModelResource() as errors on resource and made sure that errors/warnings that are anyway recorded on resource during previous load attempt don't get cleared instead
* Changed implementation of EcoreResourceUtil#loadModelResource() so as to make sure that I/O exceptions are no longer catched away but get routed along as runtime exceptions. This enabled the test for damaged files in ModelLoadManager#loadModelFilesInEditingDomain() to be implemented in a more natural way, i.e., by catching exceptions rather than checking the result of EcorePlatformUtil#loadModelRoot() for null.
* Changed EcoreResourceUtil#saveModelResource() and its callers correspondingly so as to have a consistent exception handling strategy for loading and saving of model resources
* Set  CONTINUE_AFTER_FATAL_ERROR_FEATURE to false on AbstractModelConverter as well
* Moved MetaModelDescriptorRegistry#markFileAsDirty() method to EcorePlatformUtil#markAsDirty()

Comment 3 Stephan Eberle 2011-09-28 06:10:33 EDT

(In reply to comment #1)
> Currently there is a test that fail because of my change,
> ModelSyncingTest.testAlreadyLoadedFileChanged but looking at the test I think is
> normal to fail and the test need to be reconsidered.

Actually, no. The test is right. It manipulates a project with 3 Hummingbird 2.0 model files. At the beginning the Hummingbird files are loaded and a IModelDescriptor for these files in the enclosing project is present. Then then test sets content of the Hummingbird 2.0 files to empty (i.e, no characters). This entails that the Hummingbird files can no longer be loaded and get their content type set to IExtendedPlatformConstants#CONTENT_TYPE_ID_DAMAGED_XML_FILE. However, this happens at a very late moment in time, i.e., after the unsuccessful reload attempt triggered by BasicModelSynchronizerDelegate#handleFileChanged(). Before that, the BasicModelDescriptorSynchronizerDelegate#handleFileChanged() has already attempted to update the ModelDescriptorRegistry, i.e., to remove the IModelDescriptor for the 3 Hummingbird files, but it concluded that it is to be let in place due to the fact that there still 3 files with Hummingbird 2.0 content type. When all this is over the Hummingbird files are gone because they have been marked as damaged XML files but the related IModelDescriptor is still there. This is what the test complains about.

A possible solution to this problem could consist of enhancing the EcorePlatformUtil#markAsDamaged() method and let it update the ModelDescriptorRegistry. However, I don't believe that this would be a good thing because 
* EcorePlatformUtil would end up reaching out to too many things (MetaModelDescriptorRegistry, ModelDescriptorRegistry) which it never did before
* model files, even when they are damaged, are still model files. So changing their content type to something different doesn't look appropriate from a principal point of view.

As a consequence, I have removed the EcorePlatformUtil#markAsDamaged() method reworked the ModelLoadManager#loadModelFilesInEditingDomain() method correspondingly. This lets the ModelSyncingTest#testAlreadyLoadedFileChanged() pass again and I couln't observe any negative side effect when running the technology demonstrator and loading models with damaged files in the Model Explorer view. In particular, the problem of that there are infinitely repeated load attempts of damaged files (which was meant to be addressed with the introduction of the EcorePlatformUtil#markAsDamaged()) did not occur. It has in my opinion already been addressed and fixed in bug #350901.

Comment 4 Stephan Eberle 2011-09-28 06:22:36 EDT

I've removed the code in ExtendedXMLLoadImpl and AbstractModelConverter where the parser feature CONTINUE_AFTER_FATAL_ERROR_FEATURE is set to false. This default value for this feature defined by Xerces is anyway false (see http://xerces.apache.org/xerces2-j/features.html for details).

But the main reason is a quite smart capability which we get when we don't hard-code the value of this feature to false: By simply setting the value of this feature to true in the load options, e.g., in up-front the ResourceFactoryImpl like this:

		Map<String, Boolean> parserFeatures = new HashMap<String, Boolean>();
		parserFeatures.put(Constants.XERCES_FEATURE_PREFIX + Constants.CONTINUE_AFTER_FATAL_ERROR_FEATURE, true);
		result.getDefaultLoadOptions().put(XMLResource.OPTION_PARSER_FEATURES, parserFeatures);

or later when calling EcoreResourceUtil#loadModelRoot(), we can very elegantly switch back to the previous behavior where I/O and well-formedness problems are just recorded on the resource and the loading is not aborted but continued on a best effort basis.

Comment 5 Balazs Grill 2021-07-14 02:15:12 EDT

Mass-closing Resolved tickets