Repository navigation
[ISSUE #7871] refactor(ai-proxy): introduce protocol conversion abstraction - #7910
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Review — APPROVE
I read the full 24-file diff at a1941f56, checked the new wiring against the Spring context built in AiProxyPluginConfigurationTest, and confirmed pr_build really ran this module (MODULE_ARGS="-pl shenyu-plugin/shenyu-plugin-ai/shenyu-plugin-ai-proxy,shenyu-spring-boot-starter/.../shenyu-spring-boot-starter-plugin-ai-proxy -am", ConnectResponsesToChatConverterTest 10/10, AiProtocolConverterFactoryTest 9/9, ConnectProviderDialectAdapterTest 3/3, AiProxyEngineOrchestrationTest 47/47, AiProxyPluginConfigurationTest 12/12, BUILD SUCCESS).
The separation itself is sound and I could not reproduce a functional regression. Details of what I verified, then non-blocking notes.
Verified
1. The adaptResponse hook was genuinely dead. AiProxyProvider.adaptResponse had no override in main (only OpenAi/OpenAiCompatible exist, and neither overrode it — OpenAi.prepareRequest returned the response adapter instead). Removing it and collapsing attempt() to prepared.responseAdapter().adapt(response) preserves the old order. AiProxyEngineOrchestrationTest now asserts exactly ["provider", "protocol"], which matches one provider pass + one client-protocol encode.
2. Dropping providerProfile from the reverse conversion is safe today. ConnectResponsesToChatConverter.createContext (lines 89–94) only sets toolLossPolicy and upstreamModel; the old ConnectProtocolAdapter also put profile into options.providerProfile for the same ConversionContext used by convertResponse. So the reverse pass lost the dialect. This is currently harmless: AiProxyProviderHttpTest.deepSeekChatStreamConvertsReasoningAndUsageForResponsesClients runs DeepSeek end-to-end and still asserts received.contains("trace") (from reasoning_content) plus "total_tokens":8 with no profile in the reverse context, i.e. Connect maps those fields generically. Flagging it only because the PR description says forward and reverse "share the same conversion context" and that is no longer literally true.
3. Capability gating still fails closed. chat() dropping OpenAiResponses.NAME / Operation.RESPONSE means resolveConverter always produces a Chat request for the bundled providers, and validate(providerRequest, stream) now checks the converted operation — which is what testCustomChatProviderValidatesConvertedOperation and the inverted unsupportedOperationsFailBeforeTransportWithoutEngineChanges pin down. provider.supports(providerRequest.getProtocol()) at engine line ~244 is unreachable (the factory only returns routes inside capabilities().protocols()), but it is a harmless belt-and-braces check.
4. applyModelOverride is needed and does not corrupt the payload. Without it the conversion context would have no model for the common config.model-only case, and OpenAi.prepareRequest would set the model after conversion. deepSeekChatStreamConvertsReasoningAndUsageForResponsesClients sends {"input":"hi"} with config.setModel("deepseek-flash") and would fail without it. decodeRequest deep-copies, so rawPayload is not aliased.
Non-blocking
-
Two different "upstream model" values now reach the two contexts.
createContextusesrequest.getModel()(client model, optionally overridden byconfig.model), whileConnectProviderDialectAdapterreceivesmappedModelafteroptions.modelMappings(). Before this PR both usedmappedModel. If amodelMappingsalias remaps, the Responses→Chat pass sees the alias instead of the routed model. If Connect ever starts applying model-family rules during conversion, that is a silent wrong-field risk. Cheap fix: either resolvemappedModelin the engine and pass it through, or document that the conversion context is deliberately model-agnostic. -
The default route cannot be replaced without breaking startup.
AiProtocolConverterFactory's constructor (diff line ~164) throwsIllegalArgumentException("Duplicate AI protocol converter: ...")on a duplicatesource -> target.connectResponsesToChatConverter()is only backed off by@ConditionalOnMissingBeanon the concrete type (AiProxyPluginConfiguration:134–137), so an extension that registers its ownAiProtocolConverterforopenai-responses -> openai-chatgets the default plus its own converter and the context fails to refresh. For a PR whose stated goal is an extension point, some opt-out is needed — e.g. ashenyu.plugins.ai.proxy.conversion.default-responses-to-chat=falseproperty, orputIfAbsent+ a warning instead of a hard throw. -
List<AiProtocolConverter>collection injection is order-sensitive.aiProtocolConverterFactory(...)is declared afterconnectResponsesToChatConverter()in the same class, so the default is picked up; but a converter contributed by a@Configurationprocessed later silently misses the registry (no error, just anUnsupported AI protocol conversionat request time). Worth a line in the javadoc, or injectObjectProvider<AiProtocolConverter>and resolve lazily. -
Test coverage dropped with the deleted
ConnectProtocolAdapterTest(−258). Three behaviours lost their only tests: the cumulativeMAX_CONVERSION_BYTESstream bound (the branch atConnectResponsesToChatConverter:157–170is now untested — only the complete-response limit is), the199status passthrough (< 200 || >= 300), and "stream=true withContent-Type: application/jsonmust error". The custom-tool cross-request isolation case also went away. Please port those four rather than dropping them. -
AiProxyEngine's public constructors changed signature (the 4-arg forms lostprotocol/provider). Anyone embedding the engine outside this starter gets a source break. Fine for an internal class, but it belongs in the PR description / release note. -
Double validation.
prepare()callsprovider.capabilities().validate(providerRequest, stream)(line 247) andOpenAi.prepareRequestvalidates the same request again (line 112). Pre-existing, but the new code makes the redundancy obvious. -
Minor: the "Unsupported AI upstream protocol/provider combination: X -> provider" branch (engine 243–246) is now unreachable — the factory is the only place that can reject a route, and its message (
Unsupported AI protocol conversion from X to provider protocols [Y]) is the one users will actually see. That message drops the provider name, which is the piece an operator needs; passing it intoresolveConverter, or raising the error in the engine, would be more actionable.
Unrelated, still open on master
build (shenyu-integrated-test-http) is red for everyone: #7903 changed CryptorResponsePlugin.fieldErrorParse to throw ResponsiveException(-116) while CryptorResponsePluginTest.testWhenDoesntExistFiled still asserts 10001. Not caused by this PR.
There was a problem hiding this comment.
Two additional points (@Aias00 has covered model mapping, duplicate factory routes, and test coverage):
shenyu-plugin-ai-proxy/README.mdstill references the removed/renamedConnectProtocolAdapterandAiProxyProvider.adaptResponse(around lines 46/206). It should be updated to theAiProtocolConverter/prepareRequestflow; otherwise the migration guidance for custom providers is misleading.- Removing
AiProxyProvider.adaptResponseis a silent behavior change for third-party providers: their implementations still compile, but the hook is no longer called. Consider keeping a deprecated default hook for one release, or explicitly documenting the migration in the release notes.
Otherwise I agree with @Aias00's conclusions; non-2xx passthrough matches the previous behavior.
Purpose
Part of #7871. This is the second incremental AI Proxy refactoring after #7876.
Changes
AiProtocolConverter,AiProtocolConverterFactory, and request-scopedAiProtocolConversionConnectProtocolAdapterwith a Responses-to-Chat converterAiUpstreamRequestlimited to upstream HTTP request dataOpenAI Responses native passthrough and its
/v1/responsesendpoint remain follow-up work under #7871; this PR keeps the current Engine on the Responses-to-Chat route.Tests
AiProxyPluginConfigurationTest: 12 tests passedgit diff --checkpassed