33462 generic tool paramter schema #33688

IssacAX123 · 2025-12-16T12:03:11Z

I have considered the risk of behaviour change or other zero migration impact (https://github.com/OpenLiberty/open-liberty/wiki/Behavior-Changes).
If this PR fixes an Issue, the description includes "Fixes #FILLMEIN" or "Resolves #FILLMEIN" (verify release bug label if applicable: https://github.com/OpenLiberty/open-liberty/wiki/Open-Liberty-Conventions).
If this PR resolves an external Known Issue (including APARS), the description includes "Fixes #FILLMEIN" or "Resolves #FILLMEIN".
Resolves Support MCP tool calls that contain concrete generic parameters #33462

...iberty.mcp.internal/src/io/openliberty/mcp/internal/schemas/GenericParameterDescription.java

dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/schemas/SchemaGenerator.java

devalibm

Left some comments, other than that logic seems fine to me

...openliberty.mcp.internal/src/io/openliberty/mcp/internal/typeimpl/ParameterizedTypeImpl.java

Azquelt · 2025-12-17T11:58:38Z

dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/schemas/TypeUtility.java

        return false;
    }

+    public static Type createCustomType(Type type, Map<TypeVariable<?>, Type> genericMap) {


Rename this (maybe to createResolvedType) and add Javadoc.

Azquelt · 2025-12-17T11:59:40Z

dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/schemas/TypeUtility.java

+        if (type instanceof TypeVariable<?> tv) {
+            if (genericMap.get(tv) == null) {
+                return type;
+            } else {
+                return genericMap.get(tv);
+            }


I don't recall what can go in the generic map, do we need to call it in a loop or not?

I don't think we do (based on TypeUtility.populateGenericMap)

Azquelt · 2025-12-17T13:10:03Z

....openliberty.mcp.internal/src/io/openliberty/mcp/internal/typeimpl/GenericArrayTypeImpl.java

+/**
+ *
+ */
+public class GenericArrayTypeImpl implements GenericArrayType {


Do we ever use this as a key in a map? If so we need to implement hashcode and equals.

Azquelt · 2025-12-17T13:10:19Z

...openliberty.mcp.internal/src/io/openliberty/mcp/internal/typeimpl/ParameterizedTypeImpl.java

+/**
+ *
+ */
+public class ParameterizedTypeImpl implements ParameterizedType {


If we might use this as a key in a map we need hashcode and equals.

Azquelt · 2025-12-17T13:22:10Z

dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/schemas/SchemaRegistry.java

+    public JsonObject getToolInputSchema(AnnotatedMethod<?> toolMethod, Map<TypeVariable<?>, Type> genericMap) {
        ToolKey key = new ToolKey(toolMethod, SchemaDirection.INPUT);
-        return schemaCache.computeIfAbsent(key, k -> SchemaGenerator.generateToolInputSchema(toolMethod, blueprintRegistry));
+        return schemaCache.computeIfAbsent(key, k -> SchemaGenerator.generateToolInputSchema(toolMethod, blueprintRegistry, genericMap));


This is questionable - the key should incorporate all the information required to create the schema, otherwise we will return a schema from the cache if we generate a schema for the same method but a different generic map.

I think we get away with it here because it's not valid to have two tools with the same method, since they'd automatically have the same name.

When we support multiple modules however, it might be possible.

Should I add generic Map to tool key or if you agree with this comment #33688 (comment) I could add in the argumentMaps instead

Yes, I think either of those would be fine. However, we must make sure that anything that's part of the key has equals and hashcode implemented (records have these automatically) and won't change after it's put in the map.

Azquelt · 2025-12-17T13:54:34Z

dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/ToolMetadata.java

+    private static GenericParameterDescription checkConcreteGeneric(Type baseType, Map<TypeVariable<?>, Type> genericMap, ArrayList<String> genericParams,
+                                                                    GenericParameterDescription genericParamDescription) {
+
+        List<Type> genericTypes;
+        if (baseType instanceof ParameterizedType pt) {
+            genericTypes = List.of(pt.getActualTypeArguments());
+
+        } else if (baseType instanceof GenericArrayType gat) {
+            genericTypes = List.of(gat.getGenericComponentType());
+
+        } else if (baseType instanceof TypeVariable<?> tv) {
+            genericTypes = List.of(tv);
+
+        } else if (baseType.getClass().isArray()) {
+            Type elementType = ((Class<?>) baseType).getComponentType();
+            genericTypes = List.of(elementType);
+
+        } else
+            genericTypes = List.of();
+
+        for (Type genericType : genericTypes) {
+            if (genericType instanceof TypeVariable<?> tv) {
+                if (genericMap.get(tv) == null) {
+                    genericParamDescription = GenericParameterDescription.NONCONCRETE_GENERIC;
+                } else {
+                    genericParamDescription = GenericParameterDescription.CONCRETE_GENERIC;
+                }
+            } else {
+                checkConcreteGeneric(genericType, genericMap, genericParams, genericParamDescription);
+            }
+        }
+        return genericParamDescription;
+
+    }


Suggest we change this to boolean hasUnresolvableTypeVariables(Type baseType, Map<TypeVariable<?>, Type> genericMap)

genericParams is unused and genericParamDescription is not necessary.

Azquelt · 2025-12-17T13:55:21Z

dev/io.openliberty.mcp.internal/test/io/openliberty/mcp/internal/test/schema/SchemaTest.java

        MockAnnotatedMethod<Object> toolMethod = TestUtils.findMethod(SchemaTest.class, "updateWidget");

-        String toolInputSchema = registry.getToolInputSchema(toolMethod).toString();
+        String toolInputSchema = registry.getToolInputSchema(toolMethod, new HashMap<>()).toString();


Use Collections.emptyMap() when you need an empty map that shouldn't change.

Azquelt · 2025-12-17T13:58:22Z

dev/io.openliberty.mcp.internal/test/io/openliberty/mcp/internal/test/schema/SchemaTest.java

    @Tool(name = "addGenericSingleBoundToList", title = "adds generic to generic list", description = "adds person to employee list, returns nothing")
    public @Schema(description = "Returns list of person object") <T extends Number> List<T> addGenericSingleBoundToList(@ToolArg(name = "generic list",
                                                                                                                                  description = "List of generics") List<T> list,
                                                                                                                         @ToolArg(name = "generic",
                                                                                                                                  description = "Generic object") T item) {
        list.add(item);
        return list;
        //comment
    }

-    @Test
+    @Test(expected = GenericArgumentException.class)
    public void testGenericSingleBoundToolArg() {
        MockAnnotatedMethod<Object> toolMethod = TestUtils.findMethod(SchemaTest.class, "addGenericSingleBoundToList");
-        String response = registry.getToolInputSchema(toolMethod).toString();
-        String expectedResponseString = """
-                            {
-                            "type": "object",
-                            "properties": {
-                                "generic list": {
-                                    "type": "array",
-                                    "items": {
-                                        "$ref": "#/$defs/T"
-                                    },
-                                    "description": "List of generics"
-                                },
-                                "generic": {
-                                    "$ref": "#/$defs/T",
-                                    "description": "Generic object"
-                                }
-                            },
-                            "required": [
-                                "generic list",
-                                "generic"
-                            ],
-                            "$defs": {
-                                "T": {
-                                    "type": "number"
-                                }
-                            }
-                        }
-                            """;
-        JSONAssert.assertEquals(expectedResponseString, response, true);
+        String response = registry.getToolInputSchema(toolMethod, new HashMap<>()).toString();


Huh, this used to work but now doesn't...

I think it's reasonable not to allow this.

In FAT test I got rid of all tool tests that had non concrete generic params, I forgot to remove the corresponding test in units tests.
The unit test still passed because there was different versions of getArgumentMap() calls were done, one version threw generic exception (called in ToolMetadat.createFrom())the other didnt (This one was callled in getInputTool) but in this new version they both call the same getArgumentMap which throws the exception.
There exists test which tests generic fields in objects directly using getSchema but not in getToolInput.

Azquelt · 2025-12-17T14:07:50Z

dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/ToolMetadata.java

+        } else if (baseType.getClass().isArray()) {
+            Type elementType = ((Class<?>) baseType).getComponentType();
+            genericTypes = List.of(elementType);


Suggested change

} else if (baseType.getClass().isArray()) {

Type elementType = ((Class<?>) baseType).getComponentType();

genericTypes = List.of(elementType);

} else if (baseType instanceof Class clazz && clazz.isArray()) {

Type elementType = clazz.getComponentType();

genericTypes = List.of(elementType);

Should be this I think? baseType.getClass() would return one of the subtypes of Type, whereas I think you want baseType as a Class

Azquelt · 2025-12-17T14:08:07Z

dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/ToolMetadata.java

+        } else
+            genericTypes = List.of();


Suggested change

} else

genericTypes = List.of();

} else {

genericTypes = List.of();

}

devalibm reviewed Dec 16, 2025

View reviewed changes

...iberty.mcp.internal/src/io/openliberty/mcp/internal/schemas/GenericParameterDescription.java Outdated Show resolved Hide resolved

devalibm reviewed Dec 16, 2025

View reviewed changes

dev/io.openliberty.mcp.internal/src/io/openliberty/mcp/internal/schemas/SchemaGenerator.java Outdated Show resolved Hide resolved

devalibm reviewed Dec 16, 2025

View reviewed changes

Azquelt reviewed Dec 16, 2025

View reviewed changes

...openliberty.mcp.internal/src/io/openliberty/mcp/internal/typeimpl/ParameterizedTypeImpl.java Outdated Show resolved Hide resolved

IssacAX123 added 4 commits December 16, 2025 15:30

handles nested concrete generics

7efc371

wip

2af5e4e

wip

6976df2

wip

ed3d6a7

IssacAX123 force-pushed the 33462-generic-tool-paaramter-schema branch from aa2f3fc to ed3d6a7 Compare December 16, 2025 16:04

wip

0de5f72

Azquelt requested changes Dec 17, 2025

View reviewed changes

33462 generic tool paramter schema #33688

Are you sure you want to change the base?

33462 generic tool paramter schema #33688

Conversation

IssacAX123 commented Dec 16, 2025

Uh oh!

Uh oh!

Uh oh!

devalibm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IssacAX123 Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

IssacAX123 Dec 17, 2025 •

edited

Loading