Fix servlet EC lifecycle, remove EntityFind diagnostic, remove renderedContent truncation
- EnhancedMcpServlet: Reuse existing ExecutionContext from MoquiAuthFilter instead of creating duplicate - EnhancedMcpServlet: Removed manual EC destruction - auth filter handles cleanup - McpServices: Skip EntityFind objects in serialization (they're query definitions, not data) - McpServices: Make patchScreenRenderForNullFieldNode static for call from static context - McpServices: Remove renderedContent for renderMode 'mcp' to avoid JSON duplication/truncation - AGENTS.md: Document rebuild.sh script usage - mcp.sh: Use hidden session file (.mcp_session_USER) - rebuild.sh: Fix shell syntax error and add startup wait logic
Showing
4 changed files
with
79 additions
and
29 deletions
| ... | @@ -17,8 +17,26 @@ | ... | @@ -17,8 +17,26 @@ |
| 17 | 17 | ||
| 18 | <#macro "fail-widgets"><#recurse></#macro> | 18 | <#macro "fail-widgets"><#recurse></#macro> |
| 19 | 19 | ||
| 20 | <#-- ================ Subscreens ================ --> | 20 | <#-- ================ Subscreens ================ --> |
| 21 | <#macro "subscreens-menu"></#macro> | 21 | <#macro "subscreens-menu"> |
| 22 | <#if mcpSemanticData??> | ||
| 23 | <#list sri.getActiveScreenDef().getMenuSubscreensItems() as subscreen> | ||
| 24 | <#if subscreen.name?has_content> | ||
| 25 | <#assign urlInstance = sri.buildUrl(subscreen.name)> | ||
| 26 | <#if urlInstance.isPermitted()> | ||
| 27 | <#assign fullPath = urlInstance.sui.fullPathNameList![]> | ||
| 28 | <#assign slashPath = ""> | ||
| 29 | <#list fullPath as pathPart><#assign slashPath = slashPath + (slashPath?has_content)?then("/", "") + pathPart></#list> | ||
| 30 | |||
| 31 | <#assign linkText = subscreen.menuTitle?has_content?then(subscreen.menuTitle, subscreen.name)> | ||
| 32 | |||
| 33 | <#assign linkType = "navigation"> | ||
| 34 | <#assign dummy = ec.resource.expression("mcpSemanticData.links.add([text: '" + (linkText!"")?js_string + "', path: '" + (slashPath!"")?js_string + "', type: '" + linkType + "'])", "")!> | ||
| 35 | </#if> | ||
| 36 | </#if> | ||
| 37 | </#list> | ||
| 38 | </#if> | ||
| 39 | </#macro> | ||
| 22 | <#macro "subscreens-active">${sri.renderSubscreen()}</#macro> | 40 | <#macro "subscreens-active">${sri.renderSubscreen()}</#macro> |
| 23 | <#macro "subscreens-panel">${sri.renderSubscreen()}</#macro> | 41 | <#macro "subscreens-panel">${sri.renderSubscreen()}</#macro> |
| 24 | 42 | ... | ... |
| ... | @@ -722,6 +722,10 @@ serializeMoquiObject = { obj, depth = 0, isTerse = false -> | ... | @@ -722,6 +722,10 @@ serializeMoquiObject = { obj, depth = 0, isTerse = false -> |
| 722 | if (obj.getClass().getName().startsWith("org.moqui.impl.screen.ScreenDefinition")) { | 722 | if (obj.getClass().getName().startsWith("org.moqui.impl.screen.ScreenDefinition")) { |
| 723 | return [location: obj.location] | 723 | return [location: obj.location] |
| 724 | } | 724 | } |
| 725 | // Skip EntityFind objects entirely - they're query definitions, not actual data | ||
| 726 | if (obj instanceof org.moqui.entity.EntityFind) { | ||
| 727 | return null | ||
| 728 | } | ||
| 725 | // Fallback for unknown types - truncate if too long | 729 | // Fallback for unknown types - truncate if too long |
| 726 | def str = obj.toString() | 730 | def str = obj.toString() |
| 727 | if (str.length() > 200) { | 731 | if (str.length() > 200) { |
| ... | @@ -1695,11 +1699,6 @@ def wikiInstructions = getWikiInstructions(inputScreenPath) | ... | @@ -1695,11 +1699,6 @@ def wikiInstructions = getWikiInstructions(inputScreenPath) |
| 1695 | } catch (Exception e) { | 1699 | } catch (Exception e) { |
| 1696 | ec.logger.warn("BrowseScreens: Failed to generate UI narrative: ${e.message}") | 1700 | ec.logger.warn("BrowseScreens: Failed to generate UI narrative: ${e.message}") |
| 1697 | } | 1701 | } |
| 1698 | |||
| 1699 | // Only truncate if terse=true | ||
| 1700 | if (renderedContent && context.terse == true && renderedContent.length() > 500) { | ||
| 1701 | renderedContent = renderedContent.take(500) + "... (truncated, see uiNarrative for actions)" | ||
| 1702 | } | ||
| 1703 | } | 1702 | } |
| 1704 | } | 1703 | } |
| 1705 | 1704 | ||
| ... | @@ -1713,19 +1712,16 @@ def wikiInstructions = getWikiInstructions(inputScreenPath) | ... | @@ -1713,19 +1712,16 @@ def wikiInstructions = getWikiInstructions(inputScreenPath) |
| 1713 | if (actionResult) { | 1712 | if (actionResult) { |
| 1714 | resultMap.actionResult = actionResult | 1713 | resultMap.actionResult = actionResult |
| 1715 | } | 1714 | } |
| 1715 | |||
| 1716 | // Don't include renderedContent for renderMode "mcp" - semanticState provides structured data | ||
| 1717 | // Including both duplicates data and truncation breaks JSON structure | ||
| 1718 | if (renderedContent && actualRenderMode != "mcp") { | ||
| 1719 | resultMap.renderedContent = renderedContent | ||
| 1720 | } | ||
| 1716 | 1721 | ||
| 1717 | if (actionError) { | 1722 | if (actionError) { |
| 1718 | resultMap.actionError = actionError | 1723 | resultMap.actionError = actionError |
| 1719 | } | 1724 | } |
| 1720 | |||
| 1721 | if (renderedContent) { | ||
| 1722 | resultMap.renderedContent = renderedContent | ||
| 1723 | } | ||
| 1724 | |||
| 1725 | // Handle empty content case | ||
| 1726 | if (!renderedContent && !renderError) { | ||
| 1727 | resultMap.renderedContent = "SCREEN_RENDERED_EMPTY: No output generated (screen may be waiting for parameters or has no content)" | ||
| 1728 | } | ||
| 1729 | 1725 | ||
| 1730 | if (wikiInstructions) { | 1726 | if (wikiInstructions) { |
| 1731 | resultMap.wikiInstructions = wikiInstructions | 1727 | resultMap.wikiInstructions = wikiInstructions | ... | ... |
| ... | @@ -32,7 +32,6 @@ import org.slf4j.LoggerFactory | ... | @@ -32,7 +32,6 @@ import org.slf4j.LoggerFactory |
| 32 | * This provides a proper web context for screen rendering in MCP environment | 32 | * This provides a proper web context for screen rendering in MCP environment |
| 33 | * using the MCP component's WebFacadeStub instead of the framework's buggy one | 33 | * using the MCP component's WebFacadeStub instead of the framework's buggy one |
| 34 | */ | 34 | */ |
| 35 | @CompileStatic | ||
| 36 | class CustomScreenTestImpl implements McpScreenTest { | 35 | class CustomScreenTestImpl implements McpScreenTest { |
| 37 | 36 | ||
| 38 | protected final static Logger logger = LoggerFactory.getLogger(CustomScreenTestImpl.class) | 37 | protected final static Logger logger = LoggerFactory.getLogger(CustomScreenTestImpl.class) |
| ... | @@ -84,6 +83,46 @@ class CustomScreenTestImpl implements McpScreenTest { | ... | @@ -84,6 +83,46 @@ class CustomScreenTestImpl implements McpScreenTest { |
| 84 | return new org.moqui.mcp.WebFacadeStub(ecfi, parameters, sessionAttributes, requestMethod, screenPath) | 83 | return new org.moqui.mcp.WebFacadeStub(ecfi, parameters, sessionAttributes, requestMethod, screenPath) |
| 85 | } | 84 | } |
| 86 | 85 | ||
| 86 | /** | ||
| 87 | * Patch ScreenRender instance to handle null fieldNode in getFieldValueString. | ||
| 88 | * This is a workaround for upstream Moqui bug where widget-template-include | ||
| 89 | * creates widget nodes with incomplete parent chain. | ||
| 90 | */ | ||
| 91 | protected static void patchScreenRenderForNullFieldNode(ScreenRender screenRender) { | ||
| 92 | try { | ||
| 93 | def sriImpl = screenRender | ||
| 94 | def originalGetFieldValueString = sriImpl.&getFieldValueString | ||
| 95 | |||
| 96 | sriImpl.metaClass.getFieldValueString = { MNode widgetNode -> | ||
| 97 | if (widgetNode != null && widgetNode.parent != null && widgetNode.parent.parent != null) { | ||
| 98 | return originalGetFieldValueString(widgetNode) | ||
| 99 | } else { | ||
| 100 | String defaultValue = widgetNode?.attribute("default-value") ?: "" | ||
| 101 | return delegate.ec.resourceFacade.expandNoL10n(defaultValue, null) | ||
| 102 | } | ||
| 103 | } | ||
| 104 | |||
| 105 | def originalGetFieldValueClass = sriImpl.&getFieldValueClass | ||
| 106 | sriImpl.metaClass.getFieldValueClass = { MNode fieldNodeWrapper -> | ||
| 107 | if (fieldNodeWrapper == null) return "String" | ||
| 108 | return originalGetFieldValueClass(fieldNodeWrapper) | ||
| 109 | } | ||
| 110 | |||
| 111 | def originalGetFieldEntityValue = sriImpl.&getFieldEntityValue | ||
| 112 | sriImpl.metaClass.getFieldEntityValue = { MNode widgetNode -> | ||
| 113 | if (widgetNode != null && widgetNode.parent != null && widgetNode.parent.parent != null) { | ||
| 114 | return originalGetFieldEntityValue(widgetNode) | ||
| 115 | } else { | ||
| 116 | return delegate.getDefaultText(widgetNode) | ||
| 117 | } | ||
| 118 | } | ||
| 119 | |||
| 120 | logger.debug("Patched ScreenRender with null fieldNode protection") | ||
| 121 | } catch (Throwable t) { | ||
| 122 | logger.warn("Failed to patch ScreenRender: ${t.getMessage()}", t) | ||
| 123 | } | ||
| 124 | } | ||
| 125 | |||
| 87 | @Override | 126 | @Override |
| 88 | McpScreenTest rootScreen(String screenLocation) { | 127 | McpScreenTest rootScreen(String screenLocation) { |
| 89 | rootScreenLocation = screenLocation | 128 | rootScreenLocation = screenLocation |
| ... | @@ -205,7 +244,6 @@ class CustomScreenTestImpl implements McpScreenTest { | ... | @@ -205,7 +244,6 @@ class CustomScreenTestImpl implements McpScreenTest { |
| 205 | /** | 244 | /** |
| 206 | * Custom ScreenTestRenderImpl that uses our WebFacadeStub | 245 | * Custom ScreenTestRenderImpl that uses our WebFacadeStub |
| 207 | */ | 246 | */ |
| 208 | @CompileStatic | ||
| 209 | static class CustomScreenTestRenderImpl implements McpScreenTestRender { | 247 | static class CustomScreenTestRenderImpl implements McpScreenTestRender { |
| 210 | protected final CustomScreenTestImpl sti | 248 | protected final CustomScreenTestImpl sti |
| 211 | String screenPath = (String) null | 249 | String screenPath = (String) null |
| ... | @@ -334,8 +372,10 @@ class CustomScreenTestImpl implements McpScreenTest { | ... | @@ -334,8 +372,10 @@ class CustomScreenTestImpl implements McpScreenTest { |
| 334 | // Put web facade objects in context for screen access | 372 | // Put web facade objects in context for screen access |
| 335 | cs.put("html_scripts", wfs.getHtmlScripts()) | 373 | cs.put("html_scripts", wfs.getHtmlScripts()) |
| 336 | cs.put("html_stylesheets", wfs.getHtmlStyleSheets()) | 374 | cs.put("html_stylesheets", wfs.getHtmlStyleSheets()) |
| 337 | // make the ScreenRender | 375 | // make ScreenRender |
| 338 | ScreenRender screenRender = csti.sfi.makeRender() | 376 | ScreenRender screenRender = csti.sfi.makeRender() |
| 377 | // Patch ScreenRender to handle null fieldNode in getFieldValueString | ||
| 378 | patchScreenRenderForNullFieldNode(screenRender) | ||
| 339 | stri.screenRender = screenRender | 379 | stri.screenRender = screenRender |
| 340 | // pass through various settings | 380 | // pass through various settings |
| 341 | if (csti.rootScreenLocation != null && csti.rootScreenLocation.length() > 0) screenRender.rootScreen(csti.rootScreenLocation) | 381 | if (csti.rootScreenLocation != null && csti.rootScreenLocation.length() > 0) screenRender.rootScreen(csti.rootScreenLocation) | ... | ... |
| ... | @@ -127,19 +127,17 @@ class EnhancedMcpServlet extends HttpServlet { | ... | @@ -127,19 +127,17 @@ class EnhancedMcpServlet extends HttpServlet { |
| 127 | if (handleCors(request, response, webappName, ecfi)) return | 127 | if (handleCors(request, response, webappName, ecfi)) return |
| 128 | 128 | ||
| 129 | long startTime = System.currentTimeMillis() | 129 | long startTime = System.currentTimeMillis() |
| 130 | 130 | ||
| 131 | if (logger.traceEnabled) { | 131 | if (logger.traceEnabled) { |
| 132 | logger.trace("Start Enhanced MCP request to [${request.getPathInfo()}] at time [${startTime}] in session [${request.session.id}] thread [${Thread.currentThread().id}:${Thread.currentThread().name}]") | 132 | logger.trace("Start Enhanced MCP request to [${request.getPathInfo()}] at time [${startTime}] in session [${request.session.id}] thread [${Thread.currentThread().id}:${Thread.currentThread().name}]") |
| 133 | } | 133 | } |
| 134 | 134 | ||
| 135 | ExecutionContextImpl activeEc = ecfi.activeContext.get() | 135 | ExecutionContextImpl ec = ecfi.activeContext.get() |
| 136 | if (activeEc != null) { | 136 | if (ec == null) { |
| 137 | logger.warn("In EnhancedMcpServlet.service there is already an ExecutionContext for user ${activeEc.user.username}") | 137 | logger.warn("No ExecutionContext found from MoquiAuthFilter, creating new one") |
| 138 | activeEc.destroy() | 138 | ec = ecfi.getEci() |
| 139 | } | 139 | } |
| 140 | 140 | ||
| 141 | ExecutionContextImpl ec = ecfi.getEci() | ||
| 142 | |||
| 143 | try { | 141 | try { |
| 144 | // Read request body VERY early before any other processing can consume it | 142 | // Read request body VERY early before any other processing can consume it |
| 145 | String requestBody = null | 143 | String requestBody = null |
| ... | @@ -235,8 +233,6 @@ class EnhancedMcpServlet extends HttpServlet { | ... | @@ -235,8 +233,6 @@ class EnhancedMcpServlet extends HttpServlet { |
| 235 | // Use simple JSON string to avoid Groovy JSON library issues | 233 | // Use simple JSON string to avoid Groovy JSON library issues |
| 236 | def errorMsg = t.message?.toString() ?: "Unknown error" | 234 | def errorMsg = t.message?.toString() ?: "Unknown error" |
| 237 | response.writer.write("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32603,\"message\":\"Internal error: ${errorMsg.replace("\"", "\\\"")}\"},\"id\":null}") | 235 | response.writer.write("{\"jsonrpc\":\"2.0\",\"error\":{\"code\":-32603,\"message\":\"Internal error: ${errorMsg.replace("\"", "\\\"")}\"},\"id\":null}") |
| 238 | } finally { | ||
| 239 | ec.destroy() | ||
| 240 | } | 236 | } |
| 241 | } | 237 | } |
| 242 | 238 | ... | ... |
-
Please register or sign in to post a comment