Move from com.google.refine to org.openrefine, for #226 #335

wetneb · 2023-09-24T12:37:14Z

This is the second PR in an attempt to break my work on 4.0 into reviewable chunks.

This PR moves from the com.google.refine to the org.openrefine namespace, for OpenRefine#2269.

tfmorris

I did a cursory review and transferred over some of the comments from my PR. My biggest questions/concerns:

mapping of legacy package/class names (it seemed broken to me)
loading of legacy projects which have hardcoded classnames serialized
being careful not to change legacy test data with over aggressive global replace

tfmorris · 2023-10-10T19:24:04Z

extensions/database/src/org/openrefine/extension/database/DBQueryResultImportReader.java

+import org.openrefine.extension.database.model.DatabaseColumn;
+import org.openrefine.extension.database.model.DatabaseQueryInfo;
+import org.openrefine.extension.database.model.DatabaseRow;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

-import com.google.refine.extension.database.model.DatabaseColumn;
-import com.google.refine.extension.database.model.DatabaseQueryInfo;
-import com.google.refine.extension.database.model.DatabaseRow;
-import com.google.refine.importers.TabularImportingParserBase.TableDataReader;
-import com.google.refine.importing.ImportingJob;
+import org.openrefine.importers.TabularImportingParserBase.TableDataReader;
+import org.openrefine.importing.ImportingJob;


This import ordering seems rather ad hoc. I'd suggest, as I have in the past, agreeing on a target import ordering which can then be used as part of this refactoring.

I'd be happy enforcing a certain import order, as I mentioned here: https://forum.openrefine.org/t/java-import-ordering/846
Do you know if our current linter can be configured for that, or if we should introduce a new one?
That being said, it's not clear to me at which point in the git history this linting should be done.

tfmorris · 2023-10-10T19:30:59Z

extensions/wikidata/module/MOD-INF/controller.js

@@ -5,7 +5,7 @@ importPackage(org.openrefine.wikidata.commands);
 * Function invoked to initialize the extension.
 */
 function init() {
-    var RefineServlet = Packages.com.google.refine.RefineServlet;
+    var RefineServlet = Packages.org.openrefine.RefineServlet;
    RefineServlet.registerClassMapping(
            "org.openrefine.wikidata.operations.SaveWikibaseSchemaOperation$WikibaseSchemaChange",


What is the purpose of this identity mapping? If it actually does something, an explanatory comment would be helpful, otherwise it should be removed.

If I remember correctly it's a workaround for a rather obscure class loading problem. I'd be happy to add a comment at this stage already but given that Change classes are dropped later on, it's a problem that gets solved at this point.

tfmorris · 2023-10-10T19:47:02Z

main/src/main/java/org/openrefine/RefineServlet.java

@@ -48,14 +48,16 @@ SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;

+import org.openrefine.ProjectManager;
+import org.openrefine.RefineModel;


RefineModel seems to be from a separate refactoring. I can't find where the package namespace class remapping is done any more, but I'm guessing perhaps it moved there?

It didn't appear to work in my experimentation and I also discovered that there are hardcoded classnames (!!) embedded in the serialized workspace data which caused workspace corruption for me, so this is an area that I'd want to inspect very carefully to make sure we have a robust solution in place.

Yes, RefineModel is introduced in the previous step (#334) to separate out some functionality (class loading, version information…) from the servlet. The idea being that OpenRefine's operations (and other core logic, such as facets, clustering, importers, exporters…) are exposed in an independent Maven module (or-workflow) which can be used outside of the context of a Butterfly module (which does the work of defining the web API to interact with this functionality).

tfmorris · 2023-10-10T19:53:22Z

or-model/src/main/java/org/openrefine/RefineModel.java

+        registerClassMapping("com.metaweb.refine.*", "org.openrefine.*");
+        registerClassMapping("com.google.gridworks.*", "org.openrefine.*");


These can probably be removed.

tfmorris · 2023-10-10T19:54:31Z

or-model/src/main/java/org/openrefine/RefineModel.java

-        registerClassMapping("com.google.gridworks.*", "com.google.refine.*");
+        registerClassMapping("com.metaweb.refine.*", "org.openrefine.*");
+        registerClassMapping("com.google.gridworks.*", "org.openrefine.*");
+        registerClassMapping("com.google.refine.*", "org.openrefine.*");


Does this work? In my version it couldn't read serialized JSON data with embedded com.google.refine classnames embedded in it.

I haven't looked into the issue closely given that I gave up on backwards compatibility with previous workspaces (see #335 (comment)). It's very well possible that somehow this class mapping mechanism has been broken by something else in the past and we just did not notice because it's not actually relied on unless someone is upgrading from a 10-year old version.
I could look into fixing it but I am not sure it's worth the effort, I'd rather make sure we don't use class names in workspace serialization in the new architecture (which should not be hard to reach).

I've added a commit which fixes this class name mapping functionality: ef5f47b

tfmorris · 2023-10-10T19:57:26Z

or-model/src/test/java/org/openrefine/preference/TopListTests.java


 public class TopListTests {
    @Test
    public void serializeTopList() throws JsonParseException, JsonMappingException, IOException {
        String json = "{"
-                + "\"class\":\"com.google.refine.preference.TopList\","
+                + "\"class\":\"org.openrefine.preference.TopList\","


This needs to be left with the original name to test loading legacy data. For newly written projects we need to make sure they don't include hardcoded classnames.

I've updated tests so that they point to an ostensibly old class name (com.google.gridworks) to make it clear that we're testing for backwards compatibility in ef5f47b

wetneb · 2023-10-11T06:35:44Z

Thanks for the review!

Indeed, I did not attempt to maintain compatibility with previous workspaces at all. Although that could indeed be done at this stage, I did not judge it being worth the effort given that the 4.0 version is meant to use a different workspace directory than the previous versions. That is because project serialization and architecture changes are too big to bridge in a reliable way.

I did implement later on an importer to read OpenRefine 3.x projects in 4.0, but it does so by discarding the history of the project (only importing its current state). By using different workspaces we are making sure that old projects don't get corrupted by the new version and can still be opened with the old one.

I agree that it's a bit unfortunate that class names are stored in the project serialization. The new architecture addresses this problem later on, when I removed the notion of Change entirely (whose functionality is served by Operation classes). But class names are still used in the preference store, that's something that could be worth cleaning up too. And one could want to import the preference store from the old workspace into the new one.

wetneb mentioned this pull request Sep 25, 2023

Update package prefix to org.openrefine from com.google.refine OpenRefine/OpenRefine#5952

Closed

tfmorris reviewed Oct 10, 2023

View reviewed changes

wetneb mentioned this pull request Nov 6, 2023

Normalize Java import order OpenRefine/OpenRefine#6108

Merged

Restore class name mapping functionality

ef5f47b

wetneb force-pushed the 4.0-point-2 branch from ef7cab6 to 4ed1ec9 Compare November 22, 2023 08:54

wetneb force-pushed the 4.0-point-1 branch from 00327bf to f0c9ef9 Compare November 22, 2023 08:54

wetneb added 2 commits November 25, 2023 15:03

Move from com.google.refine to org.openrefine, for OpenRefine#2269

de6d932

Fix registration of StandardReconConfig

c63e304

wetneb force-pushed the 4.0-point-2 branch from 4ed1ec9 to c63e304 Compare November 25, 2023 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from com.google.refine to org.openrefine, for #226 #335

Move from com.google.refine to org.openrefine, for #226 #335

wetneb commented Sep 24, 2023

tfmorris left a comment

tfmorris Oct 10, 2023

wetneb Oct 11, 2023

tfmorris Oct 10, 2023

wetneb Oct 11, 2023

tfmorris Oct 10, 2023

wetneb Oct 11, 2023

tfmorris Oct 10, 2023

tfmorris Oct 10, 2023

wetneb Oct 11, 2023

wetneb Nov 25, 2023

tfmorris Oct 10, 2023

wetneb Nov 25, 2023

wetneb commented Oct 11, 2023

		registerClassMapping("com.metaweb.refine.", "org.openrefine.");
		registerClassMapping("com.google.gridworks.", "org.openrefine.");

Move from com.google.refine to org.openrefine, for #226 #335

Are you sure you want to change the base?

Move from com.google.refine to org.openrefine, for #226 #335

Conversation

wetneb commented Sep 24, 2023

tfmorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wetneb commented Oct 11, 2023