Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds AI provider adapters, AI request/response abstractions, per-user token limits with time-windowed counting, pluggable usage stores (in-memory and JDBC), OpenAI-style error builders, JSON helpers, and an ObjectBinder instantiation helper. ChangesOpenAI API Integration and Rate Limiting
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ObjectBinder.java (1)
102-108: 💤 Low value
instantiatedeclares checked exceptions that are already swallowed bybind's catch-allThe helper correctly surfaces
NoSuchMethodExceptionas a descriptive error. Minor note:InvocationTargetException,InstantiationException, andIllegalAccessExceptionare declared in thethrowsclause, but sinceinstantiateisprivateand its only caller (bind) already catchesThrowable, these declarations are noise. Consider removing them, which also removes the need forbindto implicitly rely on the catch-all for those paths.🔧 Optional cleanup
- private static <T> `@NotNull` T instantiate(Class<T> clazz) throws InvocationTargetException, InstantiationException, IllegalAccessException { + private static <T> `@NotNull` T instantiate(Class<T> clazz) { try { return clazz.getConstructor().newInstance(); } catch (NoSuchMethodException e) { throw new ConfigurationParsingException("Class %s does not have a public no-arg constructor.".formatted(clazz.getName())); + } catch (InvocationTargetException | InstantiationException | IllegalAccessException e) { + throw new ConfigurationParsingException(e); } }This keeps all exception handling inside the helper and makes the contract self-contained, while
bind()'scatch (ConfigurationParsingException e)still applies the source-file context as before.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ObjectBinder.java` around lines 102 - 108, The instantiate helper should not declare InvocationTargetException, InstantiationException, or IllegalAccessException since its caller bind already swallows Throwable; instead, remove those exceptions from the method signature and catch InvocationTargetException, InstantiationException, and IllegalAccessException inside instantiate, converting them to a ConfigurationParsingException with a clear message (including clazz.getName() and the original exception as the cause); keep the existing NoSuchMethodException handling as-is; update nothing in bind beyond removing the throws dependency so bind continues to catch ConfigurationParsingException for source-file context.core/src/main/java/com/predic8/membrane/core/util/jdbc/AbstractJdbcSupport.java (1)
60-71: Open@TODO: migrate subclass connections togetConnection()The comment on line 60 documents that subclasses should be migrated to this method but haven't been yet. Without that migration the deduplication benefit is unrealised.
Would you like me to open a tracking issue for migrating existing subclasses to use
getConnection()?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/util/jdbc/AbstractJdbcSupport.java` around lines 60 - 71, Subclasses are still opening connections directly instead of using AbstractJdbcSupport.getConnection(), preventing centralized error handling and deduplication; update all subclasses that currently call datasource.getConnection() or manage SQLException to instead call getConnection() so they benefit from the ClassNotFoundException-to-ConfigurationException mapping provided by getRootCause() and ConfigurationException, remove duplicate try/catch logic from those subclasses, and ensure imports and tests reflect the consolidated behavior (look for usages of datasource.getConnection(), direct new SQLException handling, and any class-specific connection helpers to replace with getConnection()).core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiStore.java (1)
16-16: 💤 Low valueConsider documenting the
checkLimitreturn value contractIt's unclear whether the return value means "remaining tokens," "0 means deny," or something else. A brief Javadoc would help implementors (e.g.,
JDBCAiApiUsageStorecurrently returns 0 always, which may be interpreted as "limit reached").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiStore.java` at line 16, Add a clear Javadoc to the AiApiStore.checkLimit(AiApiUser user) method that defines the return-value contract (e.g., that it returns the number of remaining allowed tokens/requests, that 0 means no remaining allowance and should be treated as "deny", and any special sentinel values like -1 mean "unlimited"), and update the JDBCAiApiUsageStore implementation to conform to that contract (it currently returns 0 always which can be misinterpreted); reference the method name checkLimit and the JDBCAiApiUsageStore implementation when making the change so callers and implementors know the exact semantics.core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java (2)
91-108: 💤 Low valueOptional: replace bare
RuntimeExceptionwith a typed exception.
new RuntimeException(e)insetJsonResponse/getJsonswallows the failure category; a domain-specific exception (or at least anIllegalStateException) makes upstream handling/logging clearer and avoids mixing programmer errors with checked-IO failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java` around lines 91 - 108, Replace the bare RuntimeException throws in setJsonResponse and getJson with a more specific unchecked exception (e.g., IllegalStateException or a domain-specific exception) so failure semantics are clearer; update setJsonResponse to throw IllegalStateException with a contextual message when JsonProcessingException occurs, and update getJson to throw IllegalStateException (or a new OpenAIApiException) for both the "Expected JSON Object" case and the caught IOException, including the original exception as the cause and useful context (referencing methods setJsonResponse, getJson, om, Exchange and Flow to locate the code).
71-89: ⚡ Quick winRecommended: avoid parse/re-serialize when no transformation is needed.
getJson(line 71) andsetJsonResponse(line 87) run on every request even when bothmaxOutputTokensandmaxInputTokensare 0 (i.e., nothing to mutate). This adds latency and forces every request through this interceptor to be a JSON object — a non-JSON request will throwRuntimeExceptionfromgetJsonand surface as an internal server error. Skip the body work when there is nothing to do.♻️ Proposed shape
- var json = getJson(exc, REQUEST); - - if (maxOutputTokens != 0) { - json.put(MAX_OUTPUT_TOKENS, maxOutputTokens); - } - - if (maxInputTokens != 0) { - var input = json.get("input"); - if (input != null) { - var estimated = estimateTokens(input.asText()); - if (estimated > maxInputTokens) { - exc.setResponse(contextLengthExceeded(maxInputTokens, estimated)); - return RETURN; - } - } - } - setJsonResponse(exc, json); - return CONTINUE; + if (maxOutputTokens == 0 && maxInputTokens == 0) { + return CONTINUE; + } + var json = getJson(exc, REQUEST); + if (maxInputTokens != 0) { + var input = json.get("input"); + if (input != null) { + var estimated = estimateTokens(input.asText()); + if (estimated > maxInputTokens) { + exc.setResponse(contextLengthExceeded(maxInputTokens, estimated)); + return RETURN; + } + } + } + if (maxOutputTokens != 0) { + json.put(MAX_OUTPUT_TOKENS, maxOutputTokens); + } + setJsonResponse(exc, json); + return CONTINUE;Also note: input-token check happens after
max_output_tokensis written intojson; it's safer to validate first so a rejected request never has its body mutated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java` around lines 71 - 89, Detect when both maxOutputTokens and maxInputTokens are zero and short-circuit the interceptor before calling getJson or setJsonResponse so you skip parsing/serializing entirely; locate the logic in OpenAIAPIInterceptor.java around the getJson(...) and setJsonResponse(...) calls and return CONTINUE immediately if both limits are 0. Additionally, when maxInputTokens is > 0 or maxOutputTokens is > 0, validate the input token count first (use estimateTokens on the request input via json.get("input") and call exc.setResponse(contextLengthExceeded(...))/return RETURN if exceeded) before mutating the json with json.put(MAX_OUTPUT_TOKENS, ...), ensuring getJson is only invoked when you will actually inspect or modify the body.core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java (1)
11-11: 💤 Low valueMinor: formatting nit on
@MCElementattribute list.Missing space after the comma in
component = false, id="simple-ai-api-store"(name="simpleStore",component). Cosmetic only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java` at line 11, The `@MCElement` annotation on the SimpleAiApiStore class has a missing space after the comma in its attribute list; update the annotation on SimpleAiApiStore (the `@MCElement`(...) line) to read name="simpleStore", component = false, id="simple-ai-api-store" (ensure there is a space after the comma between attributes and keep attribute names/values unchanged) so the formatting is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/ai/AiUtil.java`:
- Around line 45-48: The current check in AiUtil that uses
ah.indexOf(BEARER_PREFIX) allows "Bearer" to appear anywhere in the header
(e.g., "XBearer ..."); change the logic to ensure the Authorization header value
starts with the Bearer scheme by using ah.trim().startsWith(BEARER_PREFIX) (or
require index == 0 after trimming) before extracting the token in the method
that currently performs the indexOf check; after confirming the prefix at the
start, strip the exact prefix and any leading whitespace to return the token.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java`:
- Line 65: The stored property MEMBRANE_AI_USERTOKEN currently holds an
Optional<AiApiUser> (set via exc.setProperty(..., user)) but is later read with
exc.getProperty(MEMBRANE_AI_USERTOKEN, String.class) for per-user tracking;
change the stored representation to the actual bearer/user token string so the
typed accessor matches: when setting MEMBRANE_AI_USERTOKEN store
user.get().getToken() (or whichever String field represents the user/bearer)
instead of the Optional, and ensure any code that reads it
(exc.getProperty(MEMBRANE_AI_USERTOKEN, String.class) and
SimpleAiApiStore.store(String user, ...)) expects that String token
consistently.
- Around line 147-149: The getAiStore() accessor currently returns null; change
it to return the configured AiApiStore instance used by the interceptor (e.g.,
return the aiStore/aiApiStore field or delegate to the existing getter used by
other accessors) so reflective access and tests see the actual store; update the
OpenAIAPIInterceptor#getAiStore method to return the interceptor's stored
AiApiStore instance (matching the field name used in the class) instead of null.
- Around line 111-126: handleResponse currently calls store.store(...)
unconditionally which causes an NPE when no aiStore is configured; update the
OpenAIAPIInterceptor.handleResponse method to guard the usage-storage logic with
a null-check (if (store != null)) before calling store.store and getUsage,
mirroring the guard used in handleRequest, and ensure you still continue
processing (return CONTINUE) when store is null; reference symbols:
handleResponse, store, store.store(...), getUsage(...), MEMBRANE_AI_USERTOKEN.
- Around line 128-134: In OpenAIAPIInterceptor.getUsage, the current calls
usage.get("input_tokens").asInt() etc. will NPE if any field is missing; change
to use the safe JSON path accessor (e.g., usage.path("input_tokens").asInt(0))
for each token field so missing keys fall back to 0, and keep the existing guard
for a completely missing usage node (return new Usage(0,0,0)). Ensure you
construct the Usage with the three safe asInt(0) results.
- Around line 120-123: The current check json.get("error").isNull() causes an
NPE when the "error" field is absent; change to a null-safe path lookup using
JsonNode.path(): obtain JsonNode error = json.path("error") and then check both
!error.isMissingNode() && !error.isNull() before returning CONTINUE (update the
code in OpenAIAPIInterceptor where json.get("error") is used).
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAiApiUtil.java`:
- Around line 25-35: contextLengthExceeded currently serializes ErrorBody
directly producing a flat JSON; change it to wrap the ErrorBody inside an
ErrorEnvelope (same pattern used by authenticationFailed() and
tokenLimitExceeded()) before calling createJson so the response shape becomes
{"error":{...}}; update the return in contextLengthExceeded to build the
badRequest().json(createJson(new ErrorEnvelope(new ErrorBody(...)))) response
using the existing ErrorEnvelope and ErrorBody classes.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiLimit.java`:
- Around line 17-38: AiApiLimit has concurrent mutations on tokens and
nextReset; make mutations atomic by synchronizing access: mark both checkLimit()
and addTokens(long) as synchronized so reads/writes to tokens and nextReset
happen under the same intrinsic lock for AiApiLimit, ensuring the reset branch
in checkLimit() and the this.tokens += tokens update are thread-safe;
alternatively, replace long tokens with AtomicLong and guard nextReset/reset
logic with a synchronized block, but keep checkLimit() and addTokens() logically
synchronized on the instance so tokens and nextReset stay consistent.
- Around line 22-24: The constructor sets nextReset using period which defaults
to 0, causing immediate resets; change initialization so nextReset is computed
lazily in checkLimit() (or validate period>0 in init()). Specifically, in class
AiApiLimit defer setting nextReset in the constructor, and in method
checkLimit() detect an uninitialized state (e.g., period <= 0 or nextReset ==
null/equals now sentinel) and then set nextReset = now().plusSeconds(period)
(after ensuring period>0) and initialize tokens appropriately; alternatively add
an init() validation in AiApiLimit to assert period > 0 and throw/configure
before any checkLimit() runs. Ensure references: AiApiLimit constructor, field
nextReset, field period, method checkLimit, and optional init().
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/JDBCAiApiUsageStore.java`:
- Around line 53-61: The methods in JDBCAiApiUsageStore — getUser(String) and
checkLimit(AiApiUser) — are stubbed and silently break
authentication/rate-limiting; replace the current bodies so they throw
UnsupportedOperationException with a clear message like "getUser not implemented
in JDBCAiApiUsageStore" / "checkLimit not implemented in JDBCAiApiUsageStore"
(or alternatively add explicit TODO comments) so callers fail fast instead of
being silently denied; ensure messages reference the method names and suggest
implementing them or using a different store, and keep the existing store()
functionality intact.
- Around line 13-22: The CREATE_TABLE_SQL in JDBCAiApiUsageStore uses
PostgreSQL's "BIGINT GENERATED ALWAYS AS IDENTITY" which breaks MySQL/MariaDB;
update createTablesIfNotExist()/JDBCAiApiUsageStore to use database-specific
DDL: detect the JDBC URL/DatabaseMetaData and emit "BIGINT AUTO_INCREMENT
PRIMARY KEY" for MySQL/MariaDB and keep the IDENTITY form for PostgreSQL, or
alternatively switch to a portable approach (e.g., remove DB identity and
generate IDs in application code) so the CREATE_TABLE_SQL constant is
chosen/constructed per database engine.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java`:
- Around line 20-33: store(...) and checkLimit(...) currently use a single
shared AiApiLimit (limit) so per-user quotas are not enforced and concurrent
updates are unsafe; change to maintain per-user limits (e.g., a
ConcurrentHashMap<String, AiApiLimit> or Map<AiApiUser, AiApiLimit> keyed by
token/username) and update store(user, usage) to add tokens to the caller's
AiApiLimit, and change checkLimit(AiApiUser) to consult that user's AiApiLimit;
also make AiApiLimit thread-safe (use AtomicLong/LongAdder for counters or
synchronize access) so addTokens/checkLimit are safe under concurrent requests,
or if you intend a global budget instead rename the field/methods to reflect a
shared limit and document it.
- Around line 16-28: The users field in SimpleAiApiStore is not initialized,
causing getUser(String) to NPE on users.stream(); fix by defensively
initializing the users field (private List<AiApiUser> users = new ArrayList<>())
or by guarding getUser(...) with a null/empty check (e.g., return
Optional.empty() if users is null) and ensure imports (java.util.ArrayList) are
added; update any related logic (store(...) and getUser(...)) to work with the
initialized/guarded users list.
---
Nitpick comments:
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ObjectBinder.java`:
- Around line 102-108: The instantiate helper should not declare
InvocationTargetException, InstantiationException, or IllegalAccessException
since its caller bind already swallows Throwable; instead, remove those
exceptions from the method signature and catch InvocationTargetException,
InstantiationException, and IllegalAccessException inside instantiate,
converting them to a ConfigurationParsingException with a clear message
(including clazz.getName() and the original exception as the cause); keep the
existing NoSuchMethodException handling as-is; update nothing in bind beyond
removing the throws dependency so bind continues to catch
ConfigurationParsingException for source-file context.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java`:
- Around line 91-108: Replace the bare RuntimeException throws in
setJsonResponse and getJson with a more specific unchecked exception (e.g.,
IllegalStateException or a domain-specific exception) so failure semantics are
clearer; update setJsonResponse to throw IllegalStateException with a contextual
message when JsonProcessingException occurs, and update getJson to throw
IllegalStateException (or a new OpenAIApiException) for both the "Expected JSON
Object" case and the caught IOException, including the original exception as the
cause and useful context (referencing methods setJsonResponse, getJson, om,
Exchange and Flow to locate the code).
- Around line 71-89: Detect when both maxOutputTokens and maxInputTokens are
zero and short-circuit the interceptor before calling getJson or setJsonResponse
so you skip parsing/serializing entirely; locate the logic in
OpenAIAPIInterceptor.java around the getJson(...) and setJsonResponse(...) calls
and return CONTINUE immediately if both limits are 0. Additionally, when
maxInputTokens is > 0 or maxOutputTokens is > 0, validate the input token count
first (use estimateTokens on the request input via json.get("input") and call
exc.setResponse(contextLengthExceeded(...))/return RETURN if exceeded) before
mutating the json with json.put(MAX_OUTPUT_TOKENS, ...), ensuring getJson is
only invoked when you will actually inspect or modify the body.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiStore.java`:
- Line 16: Add a clear Javadoc to the AiApiStore.checkLimit(AiApiUser user)
method that defines the return-value contract (e.g., that it returns the number
of remaining allowed tokens/requests, that 0 means no remaining allowance and
should be treated as "deny", and any special sentinel values like -1 mean
"unlimited"), and update the JDBCAiApiUsageStore implementation to conform to
that contract (it currently returns 0 always which can be misinterpreted);
reference the method name checkLimit and the JDBCAiApiUsageStore implementation
when making the change so callers and implementors know the exact semantics.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java`:
- Line 11: The `@MCElement` annotation on the SimpleAiApiStore class has a missing
space after the comma in its attribute list; update the annotation on
SimpleAiApiStore (the `@MCElement`(...) line) to read name="simpleStore",
component = false, id="simple-ai-api-store" (ensure there is a space after the
comma between attributes and keep attribute names/values unchanged) so the
formatting is consistent.
In
`@core/src/main/java/com/predic8/membrane/core/util/jdbc/AbstractJdbcSupport.java`:
- Around line 60-71: Subclasses are still opening connections directly instead
of using AbstractJdbcSupport.getConnection(), preventing centralized error
handling and deduplication; update all subclasses that currently call
datasource.getConnection() or manage SQLException to instead call
getConnection() so they benefit from the
ClassNotFoundException-to-ConfigurationException mapping provided by
getRootCause() and ConfigurationException, remove duplicate try/catch logic from
those subclasses, and ensure imports and tests reflect the consolidated behavior
(look for usages of datasource.getConnection(), direct new SQLException
handling, and any class-specific connection helpers to replace with
getConnection()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5199bbff-c303-4ad8-a638-e79ae07a87e6
📒 Files selected for processing (12)
annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ObjectBinder.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/AiUtil.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAiApiUtil.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiLimit.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiUser.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/JDBCAiApiUsageStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/Usage.javacore/src/main/java/com/predic8/membrane/core/security/AbstractSecurityScheme.javacore/src/main/java/com/predic8/membrane/core/util/jdbc/AbstractJdbcSupport.java
Comment on lines
+13
to
+22
| private static final String CREATE_TABLE_SQL = """ | ||
| CREATE TABLE IF NOT EXISTS ai_api_usage ( | ||
| id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY, | ||
| username VARCHAR(255) NOT NULL, | ||
| input_tokens INT NOT NULL, | ||
| output_tokens INT NOT NULL, | ||
| total_tokens INT NOT NULL, | ||
| created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP | ||
| ) | ||
| """; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does MySQL support BIGINT GENERATED ALWAYS AS IDENTITY syntax in CREATE TABLE?
💡 Result:
No, MySQL does not support the BIGINT GENERATED ALWAYS AS IDENTITY syntax in CREATE TABLE. The official MySQL documentation for versions 8.0 [1][2][3] and 8.4 [4] shows no such syntax. Instead, MySQL uses AUTO_INCREMENT for auto-generating unique values in integer columns, including BIGINT. Examples include BIGINT NOT NULL AUTO_INCREMENT PRIMARY KEY [5][6][7]. GENERATED ALWAYS AS requires an expression like (expr) [VIRTUAL|STORED], not IDENTITY [2]. This syntax is from PostgreSQL [8], and MySQL users convert it to AUTO_INCREMENT [9]. As of MySQL 8.4 (latest per release notes [4]), no support exists.
Citations:
- 1: https://dev.mysql.com/doc/refman/8.0/en/create-table.html
- 2: https://dev.mysql.com/doc/refman/8.0/en/create-table-generated-columns.html
- 3: https://dev.mysql.com/doc/en/create-table-generated-columns.html
- 4: https://dev.mysql.com/doc/relnotes/mysql/8.4/en/news-8-4-0.html
- 5: https://dev.mysql.com/doc/refman/8.1/en/example-auto-increment.html
- 6: https://www.bennadel.com/blog/4125-testing-bigint-auto-increment-usage-in-mysql-5-7-32-and-lucee-cfml-5-3-8-201.htm
- 7: https://dev.mysql.com/doc/refman/en/example-auto-increment.html
- 8: https://www.postgresql.org/docs/current/ddl-identity-columns.html
- 9: https://stackoverflow.com/questions/47637651/how-to-add-generated-by-default-as-identity-in-mysql-table
BIGINT GENERATED ALWAYS AS IDENTITY is not supported by MySQL/MariaDB
The SQL:2003 syntax GENERATED ALWAYS AS IDENTITY is specific to PostgreSQL (10+). MySQL/MariaDB do not support this; they use AUTO_INCREMENT instead. Running this DDL against MySQL will fail at startup during createTablesIfNotExist().
If multi-database support is required, use database-specific DDL generation or a more portable approach (e.g., application-layer ID generation or separate schema scripts per database).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/JDBCAiApiUsageStore.java`
around lines 13 - 22, The CREATE_TABLE_SQL in JDBCAiApiUsageStore uses
PostgreSQL's "BIGINT GENERATED ALWAYS AS IDENTITY" which breaks MySQL/MariaDB;
update createTablesIfNotExist()/JDBCAiApiUsageStore to use database-specific
DDL: detect the JDBC URL/DatabaseMetaData and emit "BIGINT AUTO_INCREMENT
PRIMARY KEY" for MySQL/MariaDB and keep the IDENTITY form for PostgreSQL, or
alternatively switch to a portable approach (e.g., remove DB identity and
generate IDs in application code) so the CREATE_TABLE_SQL constant is
chosen/constructed per database engine.
Comment on lines
+53
to
+61
| @Override | ||
| public Optional<AiApiUser> getUser(String token) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| @Override | ||
| public long checkLimit(AiApiUser user) { | ||
| return 0; | ||
| } |
Contributor
There was a problem hiding this comment.
Stubbed getUser / checkLimit make JDBCAiApiUsageStore non-functional for authentication and rate-limiting
getUser(String) always returns Optional.empty() and checkLimit(AiApiUser) always returns 0. If the interceptor uses these to authenticate callers or enforce token limits, every request will be denied when this store is configured. The store only safely records usage via store() right now.
Add at minimum a clear // TODO comment on each, or throw UnsupportedOperationException, so callers aren't silently broken.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/JDBCAiApiUsageStore.java`
around lines 53 - 61, The methods in JDBCAiApiUsageStore — getUser(String) and
checkLimit(AiApiUser) — are stubbed and silently break
authentication/rate-limiting; replace the current bodies so they throw
UnsupportedOperationException with a clear message like "getUser not implemented
in JDBCAiApiUsageStore" / "checkLimit not implemented in JDBCAiApiUsageStore"
(or alternatively add explicit TODO comments) so callers fail fast instead of
being silently denied; ensure messages reference the method names and suggest
implementing them or using a different store, and keep the existing store()
functionality intact.
- Introduced thread-safe `AtomicLong` for token management. - Synchronized reset logic in `AiApiLimit`. - Improved error handling and null checks in OpenAI API interactions. - Default-initialized user list in `SimpleAiApiStore`. - Fixed getter for `AiApiStore` in interceptor.
- Removed `AiUtil` and replaced with modular `AiProvider` interface. - Added provider implementations: `Claude`, `OpenAI`. - Updated `OpenAIAPIInterceptor` to use configurable providers and enforce model restrictions. - Introduced `NoAiApiLimit` for simplified limit management. - Enhanced error handling with model validation in `OpenAiApiUtil`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java (1)
69-69:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winProperty type mismatch breaks usage tracking (
AiApiUserstored,Stringretrieved).You store
userobject inMEMBRANE_AI_USERTOKENbut later read it asString.class; this will yield null/cast issues and wrong accounting keys.Also applies to: 137-137
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java` at line 69, You’re storing an AiApiUser object into the MEMBRANE_AI_USERTOKEN property via exc.setProperty but other code expects a String; change the set to store a String key instead (e.g., exc.setProperty(MEMBRANE_AI_USERTOKEN, user.getId() or user.getUsername()/getToken()) so the later retrieval as String.class works), and make the same change at the other occurrence around the second site (the other set at line ~137) so both storage and retrieval use the same String identifier rather than an AiApiUser instance.core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java (1)
21-34:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftQuota accounting is global, not per user.
useris ignored for limit tracking, so one heavy caller can exhaust quota for everyone. This defeats per-user enforcement.Based on learnings: verify shared-state protections are preserved when logic is split or delegated; here the shared limit state is central and should be explicitly scoped per user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java` around lines 21 - 34, The quota accounting currently uses a single shared "limit" object so store(String user, Usage usage) and checkLimit(AiApiUser user) ignore the user and apply/globalize tokens; change this to a per-user limit map (e.g., Map<String, Limit> or Map<AiApiUser, Limit>) and update store to look up the caller's Limit by token/user and call addTokens on that per-user Limit, and update checkLimit(AiApiUser user) to query that same per-user Limit; ensure the map uses a concurrent implementation (ConcurrentHashMap) or other synchronization to protect shared state and initialize a Limit for a user on first use (using get-or-create semantics) so existing methods getUser, store, and checkLimit operate per-user rather than against the single "limit" field.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/NoAiApiLimit.java (1)
3-8: ⚡ Quick win
NoAiApiLimitenforces a fixed limit of 1000 instead of "no limit".The class name implies unlimited access, but
checkLimit()returns a hardcoded 1000. SincecheckLimit()semantically represents remaining quota and is used to reject requests when the value drops to zero, this effectively caps all users at 1000 tokens. Either rename the class to reflect the actual limit (e.g.,DefaultAiApiLimit) or returnLong.MAX_VALUE(or similar unbounded sentinel) to match the "no limit" semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/NoAiApiLimit.java` around lines 3 - 8, The class NoAiApiLimit currently returns a hardcoded 1000 in checkLimit(), which contradicts its "no limit" name and caps usage; update the implementation of NoAiApiLimit.checkLimit() to return an unbounded sentinel such as Long.MAX_VALUE (or another agreed "no limit" value) so it truly represents unlimited quota, or alternatively rename the class (e.g., DefaultAiApiLimit) and keep the 1000 if the intention is a fixed default limit; reference NoAiApiLimit and its checkLimit() method (and AiApiLimit base type) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java`:
- Around line 80-89: The current logic only calls
provider.estimateInputTokens(json) if json.get("input") is present, which skips
providers that estimate from other fields; change the block in
OpenAIAPIInterceptor so that whenever maxInputTokens != 0 you call
provider.estimateInputTokens(json) unconditionally (remove the input null
check), then compare the returned estimated value to maxInputTokens and, if
exceeded, call exc.setResponse(contextLengthExceeded(maxInputTokens, estimated))
and return RETURN; ensure you keep the same symbols: maxInputTokens,
provider.estimateInputTokens(json), exc.setResponse(...),
contextLengthExceeded(...), and RETURN.
- Around line 56-57: provider is used without validation and can NPE when
calling provider.getApiKey(...) or provider.setApiKey(...); add a non-null
validation either at startup in the OpenAIAPIInterceptor.init() method (throw a
clear configuration exception if provider is null) or add a request-time guard
in the request handling path that checks if provider == null and returns a 5xx
response with a clear error message; update references to
provider.getApiKey(...) and provider.setApiKey(...) to only be executed after
this check so no NPE can occur.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/OpenAI.java`:
- Around line 35-42: In OpenAI.java, the Bearer token extraction currently uses
ah.indexOf(BEARER_PREFIX) which matches the prefix anywhere; change the logic in
the method that reads ah to require a strict prefix (e.g., after trimming
leading whitespace the header must startWith BEARER_PREFIX or match a
case‑insensitive regex like ^Bearer\s+(\S+)$) and then extract the token from
that exact location (using BEARER_PREFIX.length()), returning null for any
header that does not strictly start with the Bearer prefix or that yields an
empty token; reference BEARER_PREFIX and the variable ah when making this
change.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java`:
- Around line 27-29: getUser in SimpleAiApiStore uses u.getToken().equals(token)
which can NPE if a configured AiApiUser has a null token; change the equality
check to Objects.equals(u.getToken(), token) inside the getUser method to safely
handle nulls and add an import for java.util.Objects if not already present so
the stream filter becomes safe.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java`:
- Line 69: You’re storing an AiApiUser object into the MEMBRANE_AI_USERTOKEN
property via exc.setProperty but other code expects a String; change the set to
store a String key instead (e.g., exc.setProperty(MEMBRANE_AI_USERTOKEN,
user.getId() or user.getUsername()/getToken()) so the later retrieval as
String.class works), and make the same change at the other occurrence around the
second site (the other set at line ~137) so both storage and retrieval use the
same String identifier rather than an AiApiUser instance.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java`:
- Around line 21-34: The quota accounting currently uses a single shared "limit"
object so store(String user, Usage usage) and checkLimit(AiApiUser user) ignore
the user and apply/globalize tokens; change this to a per-user limit map (e.g.,
Map<String, Limit> or Map<AiApiUser, Limit>) and update store to look up the
caller's Limit by token/user and call addTokens on that per-user Limit, and
update checkLimit(AiApiUser user) to query that same per-user Limit; ensure the
map uses a concurrent implementation (ConcurrentHashMap) or other
synchronization to protect shared state and initialize a Limit for a user on
first use (using get-or-create semantics) so existing methods getUser, store,
and checkLimit operate per-user rather than against the single "limit" field.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/NoAiApiLimit.java`:
- Around line 3-8: The class NoAiApiLimit currently returns a hardcoded 1000 in
checkLimit(), which contradicts its "no limit" name and caps usage; update the
implementation of NoAiApiLimit.checkLimit() to return an unbounded sentinel such
as Long.MAX_VALUE (or another agreed "no limit" value) so it truly represents
unlimited quota, or alternatively rename the class (e.g., DefaultAiApiLimit) and
keep the 1000 if the intention is a fixed default limit; reference NoAiApiLimit
and its checkLimit() method (and AiApiLimit base type) when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ac0a859-8e9a-4f2a-bead-13d81b96b81b
📒 Files selected for processing (9)
core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAiApiUtil.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/AiProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/Claude.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/OpenAI.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiLimit.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/NoAiApiLimit.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java
✅ Files skipped from review due to trivial changes (2)
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiStore.java
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/AiProvider.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiLimit.java
|
This pull request needs "/ok-to-test" from an authorized committer. |
…bstraction - Added `AiApiRequest` and `AiApiResponse` abstractions for request/response handling. - Introduced `AbstractAiApiRequest` and `AbstractAiApiResponse` as base classes. - Implemented providers: `Google`, `OpenAI`, and `Claude` with concrete request/response handling. - Updated `AiProvider` to handle request/response creation. - Refactored `OpenAIAPIInterceptor` to leverage request/response abstraction and enforce contract restrictions. - Enhanced `JsonUtil` with helper methods for JSON body parsing and updates. - Updated `AiApiStore` and related classes for improved usage tracking and user abstraction.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/ai/AbstractAiApiRequest.java (1)
40-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBearer token extraction is too permissive — matches anywhere in the header value.
indexOf(BEARER_PREFIX)accepts headers like"Basic abc Bearer xyz"and extracts"xyz"as the token. The standard requires the scheme to be at the start of the value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/ai/AbstractAiApiRequest.java` around lines 40 - 47, The current extraction uses ah.indexOf(BEARER_PREFIX) which wrongly matches the scheme anywhere in the header; change the logic to require the scheme at the start (e.g., use ah.startsWith(BEARER_PREFIX) or a regex anchored with ^) before calling ah.substring(...) and trimming, and return null if the header does not begin with BEARER_PREFIX; keep using BEARER_PREFIX, ah.substring(...), and token.isEmpty() checks but only after validating the prefix is at the start.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/ClaudeAiRequest.java`:
- Around line 15-35: The method estimateInputTokens uses an int accumulator
named tokens which can overflow before being returned as a long; change the
accumulator declaration in estimateInputTokens from int to long and ensure all
constants/updates use long arithmetic (e.g., use 1000L and ensure divisions
produce long results or cast operands) when adding system, message, block and
image token estimates so no intermediate int overflow occurs when computing the
total from json.path("system") and json.path("messages").
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/GoogleAiRequest.java`:
- Around line 107-118: getGenerationConfig() can NPE because json may be null
(see estimateInputTokens guard); update getGenerationConfig() (used by
setMaxOutputTokens) to first ensure json is non-null and is an ObjectNode
(create/replace it with a new ObjectNode when null or not an object) before
calling json.get("generationConfig"), then return or create the
"generationConfig" ObjectNode; reference methods: getGenerationConfig,
setMaxOutputTokens and the field json.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/OpenAiAiRequest.java`:
- Around line 15-17: The estimateInputTokens() method in OpenAiAiRequest
currently uses json.path("input").asText("") which returns an empty string for
Chat Completions (no "input" field) and for ArrayNode inputs, making token
checks no-op; update estimateInputTokens() to inspect both "input" and
"messages": if "messages" is an array (Chat Completions) iterate it and
concatenate or sum the lengths of text-like fields (e.g., each message's
"content" or all string values) to compute total chars, if "input" is an array
iterate elements and for each element use asText("") for strings or extract
textual fields from objects before summing; finally compute tokens as
ceil(totalChars / 4.0) and return that long so OpenAIAPIInterceptor's
maxInputTokens check works correctly.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/JDBCAiApiUsageStore.java`:
- Line 15: The SQL DDL string in JDBCAiApiUsageStore (created in the
createTablesIfNotExist block) contains a Java text-block literal with a `//
`@TODO` ...` comment which is being embedded into the SQL and breaks parsing;
remove the `// `@TODO` ...` from the SQL text block or replace it with a valid SQL
comment (e.g., `/* ... */` or `-- ...`), ensuring the `id BIGINT GENERATED
ALWAYS AS IDENTITY PRIMARY KEY` line ends cleanly (no stray `//` token) so the
DDL executes successfully.
In `@core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java`:
- Around line 89-98: getJsonObject currently throws a bare
RuntimeException/IOException on non-JSON or non-object bodies which bubbles out
and crashes the pipeline; change getJsonObject to return Optional<ObjectNode>
(signature: getJsonObject(Message msg) -> Optional<ObjectNode>) and implement it
to return Optional.empty() for non-object JSON or when parsing fails (wrap IO
errors in UncheckedIOException only if you must rethrow); then update callers
(notably AbstractAiApiResponse constructor and any usages in
OpenAIAPIInterceptor) to check the Optional and convert an empty result into a
structured error response or a controlled exception (e.g.,
IllegalArgumentException) so the interceptor can map it to an HTTP error instead
of letting an unchecked RuntimeException propagate.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/AbstractAiApiRequest.java`:
- Around line 40-47: The current extraction uses ah.indexOf(BEARER_PREFIX) which
wrongly matches the scheme anywhere in the header; change the logic to require
the scheme at the start (e.g., use ah.startsWith(BEARER_PREFIX) or a regex
anchored with ^) before calling ah.substring(...) and trimming, and return null
if the header does not begin with BEARER_PREFIX; keep using BEARER_PREFIX,
ah.substring(...), and token.isEmpty() checks but only after validating the
prefix is at the start.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b118fe28-0fcb-4a38-a1db-897ea2b30c18
📒 Files selected for processing (20)
core/src/main/java/com/predic8/membrane/core/interceptor/ai/AbstractAiApiRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/AbstractAiApiResponse.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/AiApiRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/AiApiResponse.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/AiProvider.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/Claude.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/ClaudeAiRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/ClaudeAiResponse.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/Google.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/GoogleAiRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/GoogleAiResponse.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/OpenAI.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/OpenAiAiRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/OpenAiAiResponse.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiUser.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/JDBCAiApiUsageStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.javacore/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java
✅ Files skipped from review due to trivial changes (4)
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/OpenAiAiResponse.java
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/AiApiRequest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/ClaudeAiResponse.java
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiStore.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiUser.java
|
|
||
| private static final String CREATE_TABLE_SQL = """ | ||
| CREATE TABLE IF NOT EXISTS ai_api_usage ( | ||
| id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY, // @TODO GENERATED ALWAYS AS IDENTITY is PostgreSQL specific |
Contributor
There was a problem hiding this comment.
// inside the SQL text block is invalid SQL syntax — causes startup failure
Java text blocks treat // as literal string content, not a comment. The // @todo ... text is included verbatim in the DDL submitted to the database. SQL supports single-line comments with -- and multi-line comments /* */; // is not a recognized SQL comment delimiter in any major RDBMS. The DB parser will see an unexpected token after the PRIMARY KEY, comma and throw a SQL syntax error, causing createTablesIfNotExist() to throw a RuntimeException at startup.
🐛 Proposed fix
- id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY, // `@TODO` GENERATED ALWAYS AS IDENTITY is PostgreSQL specific
+ id BIGINT GENERATED ALWAYS AS IDENTITY PRIMARY KEY, -- TODO: GENERATED ALWAYS AS IDENTITY is PostgreSQL-specific; adapt for MySQL/MariaDB🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/JDBCAiApiUsageStore.java`
at line 15, The SQL DDL string in JDBCAiApiUsageStore (created in the
createTablesIfNotExist block) contains a Java text-block literal with a `//
`@TODO` ...` comment which is being embedded into the SQL and breaks parsing;
remove the `// `@TODO` ...` from the SQL text block or replace it with a valid SQL
comment (e.g., `/* ... */` or `-- ...`), ensuring the `id BIGINT GENERATED
ALWAYS AS IDENTITY PRIMARY KEY` line ends cleanly (no stray `//` token) so the
DDL executes successfully.
…ponents - Updated `checkLimit` method to consider input and output tokens. - Improved token calculation logic in AI request providers. - Enhanced JSON parsing in `JsonUtil` with Optional for safer operations. - Added detailed error handling in `OpenAIAPIInterceptor` for invalid requests. - Refined token estimation logic with safety margins and JSON structure considerations.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiLimit.java`:
- Around line 69-72: The setter setPeriod(int period) must validate that period
> 0 and must update the `@GuardedBy`("lock") field nextReset while holding the
lock; modify setPeriod to throw an IllegalArgumentException for non-positive
period values and move the assignment of nextReset = now().plusSeconds(period)
inside the same critical section that protects nextReset (use the class's lock
or synchronized(lock)) so both period and nextReset are updated atomically under
the lock.
- Around line 39-53: The reset-check and token updates must be made atomic: move
the nextReset null/now.isAfter(nextReset) check and the subsequent
tokens.set(0)/nextReset update inside the same synchronized(lock) block and
ensure any token-modifying methods use that same lock; specifically wrap the
block that computes and returns maxTokens - tokens.get() - tokensForNextRequest
in synchronized(lock) (the method containing nextReset) and change
addTokens(long tokens) to synchronize on lock as well (or perform its add inside
synchronized(lock)) so tokens, nextReset and tokensForNextRequest are always
updated/read under the same lock.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java`:
- Around line 22-23: Replace the INFO-level log that prints per-user identifiers
in SimpleAiApiStore (the log.info call that uses user.getName() and usage) with
a lower-verbosity, non-identifying message: change log.info(...) to
log.debug(...) (or log.trace(...) if you prefer) and remove the user.getName()
from the message so only non-sensitive data like usage.totalTokens() or a
redacted indicator is logged; keep the limit.addTokens(usage.totalTokens())
behavior unchanged.
- Around line 40-42: The setter setUsers currently allows assigning null to the
users field which later causes NPEs in getUser when it calls users.stream();
update setUsers to guard against null by assigning an empty immutable list
(e.g., Collections.emptyList() or List.of()) when the incoming users argument is
null, so that getUser can safely call users.stream(); keep the field name users
and the method getUser unchanged so callers remain consistent.
In `@core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java`:
- Around line 98-103: The code in JsonUtil.java reads a JsonNode with "JsonNode
jsonNode = om.readTree(msg.getBodyAsStreamDecoded())" but dereferences jsonNode
via jsonNode.getNodeType(), which can NPE when readTree returns null for empty
input; update the logic in the method containing that line to first check "if
(jsonNode == null) return Optional.empty();" (and/or log that input was empty)
before any access, and only call getNodeType() when jsonNode is non-null; also
keep the existing branch that returns Optional.of(on) when the node is an
ObjectNode and return Optional.empty() for any other node types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f29cb49e-f9a8-4c53-b7fc-24c1061b037c
📒 Files selected for processing (11)
core/src/main/java/com/predic8/membrane/core/interceptor/ai/AbstractAiApiRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/AbstractAiApiResponse.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/ClaudeAiRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/OpenAiAiRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiLimit.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/JDBCAiApiUsageStore.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/NoAiApiLimit.javacore/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.javacore/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/NoAiApiLimit.java
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/provider/ClaudeAiRequest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/AbstractAiApiRequest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/ai/OpenAIAPIInterceptor.java
Comment on lines
+39
to
+53
| if (nextReset == null || now.isAfter(nextReset)) { | ||
| synchronized (lock) { | ||
| tokens.set(0); | ||
| nextReset = now.plusSeconds(period); | ||
| log.debug("Resetting AI API usage limit."); | ||
| } | ||
| } | ||
|
|
||
| return maxTokens - tokens.get() - tokensForNextRequest; | ||
| } | ||
|
|
||
| public void addTokens(long tokens) { | ||
| log.debug("Adding {} tokens to AI API usage limit.", tokens); | ||
| this.tokens.addAndGet(tokens); | ||
| } |
Contributor
There was a problem hiding this comment.
Reset/check and token updates are not atomic across threads.
Line 39 checks nextReset outside the lock, and Line 50 updates tokens without the same lock used for reset. Under concurrent requests, this can double-reset or lose accounting around window boundaries.
🔧 Minimal locking fix
public long checkLimit(long tokensForNextRequest) {
- Instant now = now();
-
- if (nextReset == null || now.isAfter(nextReset)) {
- synchronized (lock) {
+ synchronized (lock) {
+ Instant now = now();
+ if (nextReset == null || now.isAfter(nextReset)) {
tokens.set(0);
nextReset = now.plusSeconds(period);
log.debug("Resetting AI API usage limit.");
}
+ return maxTokens - tokens.get() - tokensForNextRequest;
}
-
- return maxTokens - tokens.get() - tokensForNextRequest;
}
public void addTokens(long tokens) {
- log.debug("Adding {} tokens to AI API usage limit.", tokens);
- this.tokens.addAndGet(tokens);
+ synchronized (lock) {
+ log.debug("Adding {} tokens to AI API usage limit.", tokens);
+ this.tokens.addAndGet(tokens);
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiLimit.java`
around lines 39 - 53, The reset-check and token updates must be made atomic:
move the nextReset null/now.isAfter(nextReset) check and the subsequent
tokens.set(0)/nextReset update inside the same synchronized(lock) block and
ensure any token-modifying methods use that same lock; specifically wrap the
block that computes and returns maxTokens - tokens.get() - tokensForNextRequest
in synchronized(lock) (the method containing nextReset) and change
addTokens(long tokens) to synchronize on lock as well (or perform its add inside
synchronized(lock)) so tokens, nextReset and tokensForNextRequest are always
updated/read under the same lock.
Comment on lines
+69
to
+72
| public void setPeriod(int period) { | ||
| this.period = period; | ||
| nextReset = now().plusSeconds(period); | ||
| } |
Contributor
There was a problem hiding this comment.
period should be validated and nextReset should be written under its guard.
Line 69 accepts period <= 0, which can neutralize the limiter. Also, Line 71 writes nextReset outside the lock even though it is annotated @GuardedBy("lock").
🔧 Suggested setter hardening
`@MCAttribute`
public void setPeriod(int period) {
+ if (period <= 0) {
+ throw new IllegalArgumentException("period must be > 0");
+ }
- this.period = period;
- nextReset = now().plusSeconds(period);
+ synchronized (lock) {
+ this.period = period;
+ nextReset = now().plusSeconds(period);
+ }
}Based on learnings: “verify that the synchronization guard (the GuardedBy target …) is carried … and not dropped.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/AiApiLimit.java`
around lines 69 - 72, The setter setPeriod(int period) must validate that period
> 0 and must update the `@GuardedBy`("lock") field nextReset while holding the
lock; modify setPeriod to throw an IllegalArgumentException for non-positive
period values and move the assignment of nextReset = now().plusSeconds(period)
inside the same critical section that protects nextReset (use the class's lock
or synchronized(lock)) so both period and nextReset are updated atomically under
the lock.
Comment on lines
+22
to
+23
| log.info("User: {} Usage: {}", user.getName(), usage); | ||
| limit.addTokens(usage.totalTokens()); |
Contributor
There was a problem hiding this comment.
Avoid INFO-level logging of user usage details.
Line 22 logs per-user usage at INFO, which can leak identifiers/consumption data into default production logs.
🔧 Safer logging level/content
- log.info("User: {} Usage: {}", user.getName(), usage);
+ log.debug("Recording AI usage. totalTokens={}", usage.totalTokens());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| log.info("User: {} Usage: {}", user.getName(), usage); | |
| limit.addTokens(usage.totalTokens()); | |
| log.debug("Recording AI usage. totalTokens={}", usage.totalTokens()); | |
| limit.addTokens(usage.totalTokens()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java`
around lines 22 - 23, Replace the INFO-level log that prints per-user
identifiers in SimpleAiApiStore (the log.info call that uses user.getName() and
usage) with a lower-verbosity, non-identifying message: change log.info(...) to
log.debug(...) (or log.trace(...) if you prefer) and remove the user.getName()
from the message so only non-sensitive data like usage.totalTokens() or a
redacted indicator is logged; keep the limit.addTokens(usage.totalTokens())
behavior unchanged.
Comment on lines
+40
to
+42
| public void setUsers(List<AiApiUser> users) { | ||
| this.users = users; | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against null in setUsers to keep lookup path safe.
Line 41 can assign null, which reopens a users.stream() NPE path in getUser.
🔧 Defensive setter update
public void setUsers(List<AiApiUser> users) {
- this.users = users;
+ this.users = users != null ? users : new ArrayList<>();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void setUsers(List<AiApiUser> users) { | |
| this.users = users; | |
| } | |
| public void setUsers(List<AiApiUser> users) { | |
| this.users = users != null ? users : new ArrayList<>(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/ai/store/SimpleAiApiStore.java`
around lines 40 - 42, The setter setUsers currently allows assigning null to the
users field which later causes NPEs in getUser when it calls users.stream();
update setUsers to guard against null by assigning an empty immutable list
(e.g., Collections.emptyList() or List.of()) when the incoming users argument is
null, so that getUser can safely call users.stream(); keep the field name users
and the method getUser unchanged so callers remain consistent.
Comment on lines
+98
to
+103
| JsonNode jsonNode = om.readTree(msg.getBodyAsStreamDecoded()); | ||
| if (jsonNode instanceof ObjectNode on) { | ||
| return Optional.of(on); | ||
| } | ||
| log.debug("Expected JSON Object but got: {}",jsonNode.getNodeType()); | ||
| } catch (IOException e) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Jackson ObjectMapper.readTree(InputStream)returnnull for empty input/EOF, and what is the documented behavior?
💡 Result:
Yes for Jackson’s ObjectMapper.readTree(InputStream): for an empty/EOF input (no content available), it returns null. Documented behavior: - In the ObjectMapper javadoc, readTree(InputStream) “Returns … the root of the resulting tree … or null if no more content is accessible via passed JsonParser.” [1] - Related (implementation/design) discussion notes that end-of-input for readTree returns Java null (rather than throwing or returning a special node) and that there was historical/edge-case discussion around alignment with other readTree variants. [2][3] Practical nuance (Jackson 3.x / ObjectReader difference): - ObjectReader.readTree(InputStream) explicitly documents that its end-of-input behavior differs: it returns a “missing node” (not null). [4] So, for ObjectMapper.readTree(InputStream) specifically: empty input/EOF -> null, per javadoc. [1]
Citations:
- 1: https://javadoc.io/static/tools.jackson.core/jackson-databind/3.0.0-rc7/tools.jackson.databind/tools/jackson/databind/ObjectMapper.html
- 2:
ObjectMapper.readTree()methods do not returnnullon end-of-input FasterXML/jackson-databind#1406 - 3: Change of behavior (2.8 -> 2.9) with
ObjectMapper.readTree(input)with no content FasterXML/jackson-databind#2211 - 4: https://javadoc.io/static/tools.jackson.core/jackson-databind/3.0.1/tools.jackson.databind/tools/jackson/databind/ObjectReader.html
Handle null return from Jackson readTree() before calling getNodeType().
On line 102, jsonNode.getNodeType() will throw a NullPointerException when ObjectMapper.readTree(InputStream) returns null for empty input. Jackson's documented behavior is to return null when no content is available, breaking the intended Optional.empty() fallback.
Add a null-check before dereferencing jsonNode:
Proposed fix
public static Optional<ObjectNode> getJsonObject(Message msg) {
try {
JsonNode jsonNode = om.readTree(msg.getBodyAsStreamDecoded());
if (jsonNode instanceof ObjectNode on) {
return Optional.of(on);
}
- log.debug("Expected JSON Object but got: {}",jsonNode.getNodeType());
+ if (jsonNode != null) {
+ log.debug("Expected JSON Object but got: {}", jsonNode.getNodeType());
+ } else {
+ log.debug("Expected JSON Object but got: empty body");
+ }
} catch (IOException e) {
- log.debug("Error reading JSON: {}", e.getMessage());
+ log.debug("Error reading JSON", e);
}
return empty();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/main/java/com/predic8/membrane/core/util/json/JsonUtil.java` around
lines 98 - 103, The code in JsonUtil.java reads a JsonNode with "JsonNode
jsonNode = om.readTree(msg.getBodyAsStreamDecoded())" but dereferences jsonNode
via jsonNode.getNodeType(), which can NPE when readTree returns null for empty
input; update the logic in the method containing that line to first check "if
(jsonNode == null) return Optional.empty();" (and/or log that input was empty)
before any access, and only call getNodeType() when jsonNode is non-null; also
keep the existing branch that returns Optional.of(on) when the node is an
ObjectNode and return Optional.empty() for any other node types.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit