Merge pull request #22553 from guerler/toolrequests.000

Fix tool id handling for shed-installed tools

Author: mvdbeek

client/src/components/Form/utilities.js

@@ -194,23 +194,32 @@ function _convertValue(node, value) {
         return _convertDataValue(value, node.multiple);
     }
     if (node.type === "data_column") {
-        if (value === null || value === undefined || value === "") {
-            return value;
+        if (value === undefined) {
+            return undefined;
+        }
+        if (value === null || value === "") {
+            return null;
         }
         if (Array.isArray(value)) {
             return value.map((v) => (typeof v === "string" ? parseInt(v, 10) : v));
         }
         return typeof value === "string" ? parseInt(value, 10) : value;
     }
     if (node.type === "integer") {
-        if (value === null || value === undefined || value === "") {
-            return value;
+        if (value === undefined) {
+            return undefined;
+        }
+        if (value === null || value === "") {
+            return null;
         }
         return typeof value === "string" ? parseInt(value, 10) : value;
     }
     if (node.type === "float") {
-        if (value === null || value === undefined || value === "") {
-            return value;
+        if (value === undefined) {
+            return undefined;
+        }
+        if (value === null || value === "") {
+            return null;
         }
         return typeof value === "string" ? parseFloat(value) : value;
     }

client/src/components/Form/utilities.test.js

@@ -726,10 +726,10 @@ describe("form component utilities", () => {
             expect(buildNestedState(inputs, formData)).toEqual({ col: [1, 2, 5] });
         });
 
-        it("should preserve data_column null/empty/undefined values", () => {
+        it("should convert cleared data_column values to null but keep undefined as-is", () => {
             const inputs = [{ name: "col", type: "data_column" }];
             expect(buildNestedState(inputs, { col: null })).toEqual({ col: null });
-            expect(buildNestedState(inputs, { col: "" })).toEqual({ col: "" });
+            expect(buildNestedState(inputs, { col: "" })).toEqual({ col: null });
             expect(buildNestedState(inputs, { col: undefined })).toEqual({ col: undefined });
         });
 
@@ -744,10 +744,10 @@ describe("form component utilities", () => {
             expect(buildNestedState(inputs, { num: "42" })).toEqual({ num: 42 });
         });
 
-        it("should preserve integer null/empty/undefined values", () => {
+        it("should convert cleared integer values to null but keep undefined as-is", () => {
             const inputs = [{ name: "num", type: "integer" }];
             expect(buildNestedState(inputs, { num: null })).toEqual({ num: null });
-            expect(buildNestedState(inputs, { num: "" })).toEqual({ num: "" });
+            expect(buildNestedState(inputs, { num: "" })).toEqual({ num: null });
             expect(buildNestedState(inputs, { num: undefined })).toEqual({ num: undefined });
         });
 
@@ -761,10 +761,10 @@ describe("form component utilities", () => {
             expect(buildNestedState(inputs, { val: "3.14" })).toEqual({ val: 3.14 });
         });
 
-        it("should preserve float null/empty/undefined values", () => {
+        it("should convert cleared float values to null but keep undefined as-is", () => {
             const inputs = [{ name: "val", type: "float" }];
             expect(buildNestedState(inputs, { val: null })).toEqual({ val: null });
-            expect(buildNestedState(inputs, { val: "" })).toEqual({ val: "" });
+            expect(buildNestedState(inputs, { val: "" })).toEqual({ val: null });
             expect(buildNestedState(inputs, { val: undefined })).toEqual({ val: undefined });
         });
 

lib/galaxy/celery/tasks.py

@@ -92,9 +92,14 @@ def cached_create_tool_from_representation(
     raw_tool_source: str,
     tool_source_class: TOOL_SOURCE_CLASS,
     tool_dir: Optional[str] = None,
+    tool_id: Optional[str] = None,
 ):
     return create_tool_from_representation(
-        app=app, raw_tool_source=raw_tool_source, tool_dir=tool_dir, tool_source_class=tool_source_class
+        app=app,
+        raw_tool_source=raw_tool_source,
+        tool_dir=tool_dir,
+        tool_source_class=tool_source_class,
+        guid=tool_id,
     )
 
 
@@ -441,6 +446,7 @@ def queue_jobs(request: QueueJobs, app: MinimalManagerApp, job_submitter: JobSub
         raw_tool_source=raw_tool_source,
         tool_dir=request.tool_source.tool_dir,
         tool_source_class=tool_source_class,
+        tool_id=request.tool_source.tool_id,
     )
 
     job_submitter.queue_jobs(

lib/galaxy/schema/tasks.py

@@ -182,6 +182,7 @@ class ToolSource(Model):
     raw_tool_source: str
     tool_dir: Optional[str] = None
     tool_source_class: TOOL_SOURCE_CLASS = "XmlToolSource"
+    tool_id: Optional[str] = None
 
 
 class QueueJobs(Model):

lib/galaxy/tool_util/parameters/convert.py

@@ -481,12 +481,16 @@ def encode_element(element: dict):
 
     def encode_callback(parameter: ToolParameterT, value: Any):
         if isinstance(parameter, DataParameterModel):
+            if value is None:
+                return VISITOR_NO_REPLACEMENT
             if parameter.multiple and isinstance(value, list):
                 return list(map(encode_element, value))
             else:
                 assert isinstance(value, dict), str(value)
                 return encode_element(value)
         elif isinstance(parameter, DataCollectionParameterModel):
+            if value is None:
+                return VISITOR_NO_REPLACEMENT
             assert isinstance(value, dict), str(value)
             return encode_element(value)
         else:

lib/galaxy/tools/__init__.py

@@ -469,10 +469,14 @@ def create_tool_from_source(app, tool_source: ToolSource, config_file: Optional[
 
 
 def create_tool_from_representation(
-    app, raw_tool_source: str, tool_dir: Optional[StrPath] = None, tool_source_class="XmlToolSource"
+    app,
+    raw_tool_source: str,
+    tool_dir: Optional[StrPath] = None,
+    tool_source_class="XmlToolSource",
+    guid: Optional[str] = None,
 ) -> "Tool":
     tool_source = get_tool_source(tool_source_class=tool_source_class, raw_tool_source=raw_tool_source)
-    return create_tool_from_source(app, tool_source=tool_source, tool_dir=tool_dir)
+    return create_tool_from_source(app, tool_source=tool_source, tool_dir=tool_dir, guid=guid)
 
 
 class NullToolTagManager(AbstractToolTagManager):

lib/galaxy/webapps/galaxy/services/jobs.py

@@ -282,6 +282,7 @@ def create(self, trans: ProvidesHistoryContext, job_request: JobRequest) -> JobC
             raw_tool_source=tool_source_model.source,
             tool_dir=tool.tool_dir,
             tool_source_class=tool_source_model.source_class,
+            tool_id=tool.id,
         )
         task_request = QueueJobs(
             user=trans.async_request_user,

test/unit/app/tools/test_tool_serialization_roundtrip.py

@@ -36,6 +36,23 @@ def test_repopulate_after_serialization_yaml():
     )
 
 
+def test_repopulate_applies_guid():
+    tool = simple_constructs_tool()
+    raw_tool_source, tool_source_class = tool.to_raw_tool_source()
+    guid = "toolshed.example.com/repos/owner/repo/simple_constructs_y/1.0"
+
+    app = mock_app_for_tool_support()
+    rebuilt = create_tool_from_representation(
+        app,
+        raw_tool_source,
+        tool.tool_dir,
+        tool_source_class,
+        guid=guid,
+    )
+    assert rebuilt.id == guid
+    assert rebuilt.old_id == "simple_constructs_y"
+
+
 def simple_constructs_tool() -> Tool:
     tool_path = functional_test_tool_path("simple_constructs.yml")
     tool_source = functional_test_tool_source("simple_constructs_y")

test/unit/tool_util/test_parameter_convert.py

@@ -121,6 +121,14 @@ def test_multi_data():
     assert encoded_state.input_state["parameter"][1]["id"] == EXAMPLE_ID_2_ENCODED
 
 
+def test_encode_optional_data_collection_none():
+    tool_source = tool_source_for("parameters/gx_data_collection_optional")
+    bundle = input_models_for_tool_source(tool_source)
+    internal_state = RequestInternalToolState({"parameter": None})
+    encoded_state = encode(internal_state, bundle, _fake_encode)
+    assert encoded_state.input_state["parameter"] is None
+
+
 def test_landing_encode_data():
     tool_source = tool_source_for("parameters/gx_data")
     bundle = input_models_for_tool_source(tool_source)