Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 341481

Summary:	[EMC] EmfModel#load(StringProperties, String) and clients should make same assumptions about model/metamodel locations
Product:	[Modeling] Epsilon	Reporter:	Louis Rose <louis>
Component:	Core	Assignee:	Louis Rose <louis>
Status:	CLOSED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	arcanefoam, dkolovos, ivo
Version:	unspecified
Target Milestone:	---
Hardware:	Macintosh
OS:	Mac OS X
Whiteboard:	interim

Description Louis Rose

2011-03-31 09:25:58 EDT

There are several clients of EmfModel#load(StringProperties, String), such as EmfModelConfigurationDialog and LoadModelTask. The clients pass different types of location for models and metamodels, such as fully-qualified URIs (e.g. starting with "file://" or "platform:/resource"), platform resource relative paths (e.g. "/MyProject/my.model") and absolute system paths (e.g. "/tmp/my.model" or "C:\my.model"). In EmfModel#load(StringProperties, String), we attempt to infer the most appropriate type of location from the string passed by the client. In particular, we've tried to infer whether an unqualified location should be interpreted as a platform resource URI or as a file URI.

We've tried lots of tricks in the past, such as assuming that when basePath is null, the client is indicating that the path should be interpreted as a file URI. However, we've often forgotten to fix all of the clients of EmfModel#load(StringProperties, String) when changing the assumptions made by EmfModel. This has led to regressions, such as #280056 and #285559

To try to avoid these issues in the future, we can:
  (1) change EmfModel such that its clients provide a fully-qualified URI
  (2) add regression tests for EmfModel#load(StringProperties, String)
  (3) document the new protocol for using EmfModel#load(StringProperties, String)

Comment 1 Louis Rose

2011-04-04 12:40:22 EDT

The following clients use EmfModel#load(StringProperties, String):

- EugeniaActionDelegate
- AbstractECoreModelAction
- NewEmfModelWizard
- LoadModel
- LoadModelTask*
- EclipseContextManager*

The starred clients retrieve property keys from data stored by the Epsilon user, such as in an Eclipse launch configuration or in an ANT build file. In order not to break existing user data, we must continue to support the existing loading protocol, which can infer a fully qualified URI when an unqualified URI is supplied.

Comment 2 Louis Rose

2011-04-04 12:43:12 EDT

(In reply to comment #1)
> The following clients use EmfModel#load(StringProperties, String):

There is also:

- ModelManager (EglServlet)

Comment 3 Louis Rose

2011-04-04 12:43:38 EDT

Accidentally closed bug.

Comment 4 Louis Rose

2011-04-05 10:13:42 EDT

For the clients that internally construct values for the StringProperties argument, I have migrated the code to the new protocol. For example, the following code in EugeniaActionDelegate:

	properties.put(EmfModel.PROPERTY_MODEL_FILE, path);
	properties.put(EmfModel.PROPERTY_METAMODEL_URI, nsUri);
	properties.put(EmfModel.PROPERTY_IS_METAMODEL_FILE_BASED, "false");
	properties.put(EmfModel.PROPERTY_READONLOAD, readOnLoad + "");
	properties.put(EmfModel.PROPERTY_STOREONDISPOSAL, storeOnDisposal + "");
	properties.put(EmfModel.PROPERTY_EXPAND, expand + "");
	properties.put(EmfModel.PROPERTY_NAME, name);
	
became:

	properties.put(EmfModel.PROPERTY_MODEL_URI, org.eclipse.emf.common.util.URI.createURI(path, true));
	properties.put(EmfModel.PROPERTY_METAMODEL_URI, nsUri);
	properties.put(EmfModel.PROPERTY_IS_METAMODEL_FILE_BASED, "false");
	properties.put(EmfModel.PROPERTY_READONLOAD, readOnLoad + "");
	properties.put(EmfModel.PROPERTY_STOREONDISPOSAL, storeOnDisposal + "");
	properties.put(EmfModel.PROPERTY_EXPAND, expand + "");
	properties.put(EmfModel.PROPERTY_NAME, name);

Comment 5 Louis Rose

2011-04-05 10:49:55 EDT

For the clients that construct StringProperties from user data (EclipseContextManager and LoadModelTask), we will need to support the old properties (PROPERTY_MODEL_FILE and PROPERTY_METAMODEL_FILE). To this end, I have:

- Updated EmfModelConfigurationDialogue to ensure that new launch configurations use the new properties.
- Introduced EmfModel.PropertyMigrator, which inspects an instance of StringProperties, and migrates any legacy properties to their new equivalents.

I've not changed LoadModelTask which now relies on EmfModel.PropertyMigrator for executing loadModel ANT statements that use the legacy properties. Migrating the legacy properties in LoadModelTask would have involved coupling LoadModelTask to EmfModel, which did not seem sensible as LoadModelTask does not currently need to know which implementation of IModel it is instantiating.

Comment 6 Louis Rose

2011-04-05 11:03:48 EDT

A quick summary of the most important changes:

- EmfModel's PROPERTY_MODEL_FILE and PROPERTY_METAMODEL_FILE have been deprecated in favour of PROPERTY_MODEL_URI and PROPERTY_FILE_BASED_METAMODEL_URI
- EmfModel.PropertyMigrator has been introduced so that uses of the deprecated properties will continue to be supported
- All of our code that uses EmfModel#load(StringProperties, String) has been updated to use the new rather than the old properties. EmfModelConfigurationDialogue continues to rely on the deprecated properties so as to support legacy launch configurations.

Comment 7 Louis Rose

2011-04-05 11:57:24 EDT

I've commited the first round of changes, described in all of the comments above, to SVN.

Comment 8 Ivo(sh) Musil

2011-04-05 17:56:12 EDT

Louis, you probably solved my older bug#329311 ... I will test your patch and report result.

Thanks.

Comment 9 Louis Rose

2011-04-07 07:05:14 EDT

(In reply to comment #8)
> Louis, you probably solved my older bug#329311 ... I will test your patch and
> report result.
> 
> Thanks.

Hi Ivo. Great -- that's a happy coincidence. Do let me know if this patch fixes bug#329311 and I'll mark it as a duplicate.

Comment 10 Louis Rose

2011-04-07 07:06:53 EDT

I've tested the changes on Windows and Mac OS, and there seem to be no regressions.

I also committed a small patch to ensure that EmfModel does not migrate legacy properties that are paired with the empty string. Some existing launch configurations will use pair a legacy property with the empty string because the user has "cleared" a text box that previously contained a value.

Comment 11 Louis Rose

2011-04-28 08:31:04 EDT

I think we're done with this for now. Assigning to Dimitris for build integration.

Comment 12 Louis Rose

2011-06-01 14:22:14 EDT

This is fixed in SVN, and will make it into the next interim/stable builds.

Comment 13 Dimitris Kolovos

2011-07-25 08:17:13 EDT

Fixed in 0.9.1

Comment 14 Horacio Hoyos

2014-12-13 16:33:47 EST

Hi,

My implementation of the new Bibtex driver extends the EMF model. Probably in the future other drivers that will relay in an EMF model behind the curtins may need to extend the EMF model too. 

PropertyMigrator class was introduced for backwards-compatibility with existing Eclipse launch configurations. However, this class is private and hence not visible for extending classes. 

If new drivers are to support old configurations (perhaps not) then this class should be public. If not, then it should remain private and all new drivers use the new property style.

Comment 15 Dimitris Kolovos

2014-12-14 17:44:46 EST

New drivers should not support the old configuration options in my view.

Comment 16 Dimitris Kolovos

2015-01-14 18:53:38 EST

Re-closing.

Comment 17 Dimitris Kolovos

2016-11-06 14:14:14 EST

Fixed in 1.4