Skip to content

fix(dotAI): Dot AI LangChain4J - ProviderConfig fixes#35426

Closed
ihoffmann-dot wants to merge 22 commits intomainfrom
dot-ai-langchain-fixes
Closed

fix(dotAI): Dot AI LangChain4J - ProviderConfig fixes#35426
ihoffmann-dot wants to merge 22 commits intomainfrom
dot-ai-langchain-fixes

Conversation

@ihoffmann-dot
Copy link
Copy Markdown
Member

@ihoffmann-dot ihoffmann-dot commented Apr 22, 2026

Summary

Consolidates AI configuration by moving prompts and per-capability settings into the providerConfig JSON sections, enabling a single-field setup in the Apps UI. Also adds comma-separated model fallback so any provider can specify multiple models in priority order.

  • Make providerConfig visible in the Apps UI (hidden: false) and remove legacy per-field secrets (rolePrompt, textPrompt, imagePrompt, imageSize, listenerIndexer) from dotAI.yml
  • AppConfig now reads prompts and settings from inside the providerConfig JSON sections instead of from separate app secrets
  • ProviderConfig.allModels(): parse comma-separated model field into a priority list for fallback

Configuration

{
  "chat": {
    "provider": "openai",
    "apiKey": "sk-...",
    "model": "gpt-4o,gpt-4o-mini",
    "maxTokens": 16384,
    "temperature": 1.0,
    "rolePrompt": "You are dotCMSbot, an AI assistant to help content creators.",
    "textPrompt": "Use Descriptive writing style."
  },
  "embeddings": {
    "provider": "openai",
    "apiKey": "sk-...",
    "model": "text-embedding-ada-002",
    "listenerIndexer": { "default": "blog,news,webPageContent" }
  },
  "image": {
    "provider": "openai",
    "apiKey": "sk-...",
    "model": "dall-e-3",
    "size": "1792x1024",
    "imagePrompt": "Use 16:9 aspect ratio."
  }
}

Notes

  • Prompts (rolePrompt, textPrompt, imagePrompt) and embedding settings (listenerIndexer) now live inside providerConfig sections. Separate App secrets for these fields are no longer supported.
  • Multi-model fallback: set "model": "gpt-4o,gpt-4o-mini" to try models in order. Sync requests fall back on both initialization and runtime failures; streaming falls back on initialization failures only (cannot retry once bytes are written to the output stream).

Related Issue

feat(dotAI): Dot AI LangChain4J integration (First provider/OpenAI)
#35150

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

Claude finished @ihoffmann-dot's task in 2m 44s —— View job


PR Review

  • Gather context
  • Read changed files
  • Analyze and post review

Issues

1. resolvedProviderConfig serializes credentials — AppConfig.java:55

AppConfig implements Serializable. The new resolvedProviderConfig field is not transient, so it will be written to disk if this object is ever serialized (Ehcache, cluster replication, etc.). resolvedProviderConfig contains the top-level apiKey injected into every section. Before this PR, a user who kept their API key in the dedicated App Secret field (and left it out of providerConfig) had no credentials in the serialized providerConfig string. After this PR, that same user has their key in resolvedProviderConfig which IS serialized. Mark it transient and recompute on demand, or rebuild it inside readObject(). Fix this →

2. injectApiKeyIntoSections injects OpenAI credentials into non-OpenAI sections — AppConfig.java:396

The method iterates over chat, embeddings, image unconditionally and injects the top-level apiKey into any section that doesn't have one. If a user configures Bedrock or Vertex AI, their section gets an apiKey field that shouldn't be there. It also means credentials are written into sections of unrelated providers. The injection should check provider first.

3. Streaming runtime failures don't attempt fallback — LangChain4jAIClient.java:167-183

initStreamingModel correctly tries each model on init failure. But if init succeeds and streamWithModel raises onError (network drop, model rejection, etc.) before any bytes are written to output, the exception propagates without trying the next model. The sync path via executeWithFallback handles this correctly. Whether it's safe to retry a streaming failure "before first byte" depends on whether output is observable, but right now the code doesn't distinguish — all streaming runtime failures are hard stops.

4. Failed model instances stay in cache after runtime errors — LangChain4jAIClient.java:336-357

When executor.apply(model) throws (line 352), the model instance stays in the Guava cache. The next request (within the 1-hour TTL) retrieves the same broken instance. Init failures are protected because Cache.get() doesn't cache exceptions — but runtime failures aren't. If a model repeatedly fails at runtime (e.g., bad credentials stored in the instance), callers pay the full fallback cost every time but the cache never evicts. Consider calling modelCache.invalidate(cacheKey) on runtime failure.

5. dotAI.yml hint contradicts PR body examples

The hint in dotAI.yml (line 27) says: "Do not include apiKey here — use the API Key field above. It is injected automatically." But the PR body's configuration example includes "apiKey": "sk-..." inline in every section. Users following the PR docs will embed credentials directly in providerConfig, bypassing the secure dedicated field entirely. The PR body example should be updated to omit apiKey from the JSON sections.

6. Reasoning-model regex is brittle — LangChain4jModelFactory.java:121

if (model.matches("o[0-9].*")) {

String.matches() in Java anchors both ends, so this works for "o1", "o3", "o4-mini". But it breaks for "o1-preview" — wait, "o1-preview".matches("o[0-9].*")"o" + "1" + "-preview" → matches (.* matches -preview). Actually that is fine. The real risk is that this hardcodes OpenAI's current model naming convention. If OpenAI releases "o10" or changes naming, the wrong token-limit parameter is sent and the request silently truncates or fails. This should at minimum be externalizable via a Config property. At minimum add a comment explaining why this heuristic is correct today.

7. allModels() is not cached — ProviderConfig.java:70

allModels() is a default interface method that splits and copies the model list on every call. executeWithFallback calls it once so it's fine today, but executeStreamingChatRequest also calls it independently. Not a correctness issue, just worth noting if this interface is called in hot paths.


The REST contract removal flagged in the previous comment (removing rolePrompt, textPrompt, etc. from GET /api/v1/completions/config) stands as documented before. No other structural issues.

@github-actions github-actions Bot added the Area : Backend PR changes Java/Maven backend code label Apr 22, 2026
@ihoffmann-dot ihoffmann-dot changed the title fix(dotAI): Dot AI LangChain4J - Config consolidation and multi-model fallback fix(dotAI): Dot AI LangChain4J - ProviderConfig fixes Apr 22, 2026
@dotCMS dotCMS deleted a comment from github-actions Bot Apr 22, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 23, 2026

Pull Request Unsafe to Rollback!!!

  • Category: M-3 — REST / GraphQL / Headless API Contract Change
  • Risk Level: 🟡 MEDIUM
  • Why it's unsafe: Five fields are removed from the GET /api/v1/completions/config REST response without a deprecation period. Any client (admin UI, integration, Postman tests) that already reads rolePrompt, textPrompt, imagePrompt, imageSize, or listenerIndexer from the config response will silently get undefined/null after N is deployed. On rollback to N-1 those fields return, but client code updated against N's contract (expecting them inside providerConfig) breaks in the other direction.
  • Code that makes it unsafe: dotCMS/src/main/java/com/dotcms/ai/rest/CompletionsResource.java lines 183–187 (the removal block):
-        map.put(AppKeys.ROLE_PROMPT.key, appConfig.getRolePrompt());
-        map.put(AppKeys.TEXT_PROMPT.key, appConfig.getTextPrompt());
-        map.put(AppKeys.IMAGE_PROMPT.key, appConfig.getImagePrompt());
-        map.put(AppKeys.IMAGE_SIZE.key, appConfig.getImageSize());
-        map.put(AppKeys.LISTENER_INDEXER.key, appConfig.getListenerIndexer());

The Postman test in AI.postman_collection.json is updated in the same PR to stop asserting these properties, confirming the removal is intentional and breaks previously-passing contract assertions.

  • Alternative (if possible): Keep the removed fields in the response for one release using their default values (read from providerConfig or fall back to AppKeys defaults) so N-1 clients are not broken on rollback. Remove them in the following release once N-1 is outside the rollback window. Example:
// Transitional — emit legacy keys from providerConfig values for one release
map.put(AppKeys.ROLE_PROMPT.key, appConfig.getRolePrompt());

Logger.warn(LangChain4jAIClient.class,
"Streaming model '" + modelName + "' init failed: " + cause.getMessage()
+ (models.size() > 1 ? " — trying next model" : ""));
continue;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this continue and return make harder the reading
also you should introduce some observability here, logs are ok but are not telling anything about how long it is take, etc
also you are using system prompt that never change, probably want to introduce a cache entry point to save tokens

Copy link
Copy Markdown
Member Author

@ihoffmann-dot ihoffmann-dot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I extracted the logic into initStreamingModel() and added timing logs. Regarding the cache, is a good observation but it would need some logic that we could implement in another PR.

for (final String modelName : models) {
try {
final ProviderConfig modelConfig = ImmutableProviderConfig.copyOf(baseConfig).withModel(modelName);
final M model = modelCache.get(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for instance this method is easier to read

*/
default List<String> allModels() {
final String m = model();
if (m == null || m.isBlank()) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually if something may be null, Optional is a fantastic choice

@github-actions
Copy link
Copy Markdown
Contributor

Comment on lines +168 to +169

@Test
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding javadoc to tests. We generally use this format:

  /**
     * Method to test: {@link StoryBlockAPI#refreshReferences(Contentlet)}
     * Given Scenario: This will create 2 block contents, adds a rich content to each block content and retrieve the json.
     * ExpectedResult: The new json will contain the rich text data map for each block content.
     */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants