Repository navigation
fix: namespace JSON schemas per OCPP version so they stop shadowing each other on a shared classpath - #112
Conversation
…N schemas per OCPP version OcppJsonValidator resolves schemas as classpath resources by action name, and the 1.5, 1.6 and 2.0 modules all shipped theirs at the resources root under overlapping names (57 shared between 1.6 and 2.0, 24 between 1.5 and 1.6). On a shared classpath such as toolkit, getResourceAsStream returns the oldest jar's file, so a valid OCPP 2.0.1 Authorize request was rejected against the 1.6 AuthorizeRequest schema. Move each module's schemas (and their licence notice) into ocpp15/, ocpp16/ and ocpp20/, and pass the folder from the parsers, as ocpp-1-2-json already does with ocpp12/. ocpp-1-6-security ships no schemas of its own; the security-whitepaper ones live in ocpp-1-6-json and follow it. Add JsonSchemaIsolationTest in toolkit, the only place the collision is observable: one payload per version, each valid in its own version but rejected by the older schema that used to shadow it. Fixes IZIVIA#111
Now that every version module namespaces its schemas, the empty-folder default only leaves the classpath collision open for a future module. Make schemaFolder mandatory and reject blank values at construction. BREAKING CHANGE: OcppJsonValidator(specVersion) no longer compiles; pass the resource folder holding the schemas, e.g. OcppJsonValidator(SpecVersion.VersionFlag.V4, "ocpp16").
pbourseau
left a comment
pbourseau
left a comment
There was a problem hiding this comment.
Reviewed with the branch checked out; all claims below were checked by running the builds.
Verification I could reproduce
- The bug is real and the new test catches it: applying
JsonSchemaIsolationTest+ thetoolkitbuild change ontodevgives 4 tests, 2 failures (the 2.0 and 1.6 cases). All 4 pass on this branch. :ocpp-1-2-json,:ocpp-1-5-json,:ocpp-1-6-json,:ocpp-2-0-json,:ocpp-1-6-securitygreen (328 tests, 0 failures);:ocpp-jsonand:toolkitgreen.- The
git mvlost nothing: 36 / 48 / 79 / 127 resource files before and after per module, identical basenames, and no file left at asrc/main/resourcesroot anywhere in the repo.
The diagnosis and the fix look right to me. Inline notes below, plus two points that have no diff line to hang on:
ocpp-json/build.gradle.kts still declares testImplementation(kotlin("test-junit")). ocpp-json/src/test was empty before this PR, so this is the first code to land in that source set. :ocpp-json:dependencies --configuration testRuntimeClasspath resolves kotlin-test-junit:2.4.0 -> junit:junit:4.13.2, and the runner is useJUnitPlatform() with junit-jupiter-engine but no junit-vintage-engine. A contributor who writes kotlin.test.Test or org.junit.Test in this module gets a test that compiles and silently never runs. coreProject() already supplies jupiter-api, jupiter-params, strikt and mockk, so the line can go.
Description nit: it says the OCA licence notice moves into ocpp15/, ocpp16/ and ocpp20/. There is only one in ocpp-1-6-json and ocpp-2-0-json; ocpp-1-5-json has none (and had none before).
| private val schemaFolder: String | ||
| ) { | ||
| init { | ||
| require(schemaFolder.isNotBlank()) { "schemaFolder must name the resource folder holding the schemas" } |
There was a problem hiding this comment.
isNotBlank() leaves a gap that bites exactly where it is hardest to notice: a trailing slash or surrounding whitespace passes this guard.
OcppJsonValidator(V4, "ocpp16/") builds ocpp16//Authorize.json. I checked this on a JVM with a minimal jar and an exploded copy of the same tree:
--- exploded directory on classpath ---
[ocpp16/Authorize.json] -> FOUND
[ocpp16//Authorize.json] -> FOUND
[ ocpp16 /Authorize.json] -> null
--- jar on classpath ---
[ocpp16/Authorize.json] -> FOUND
[ocpp16//Authorize.json] -> null
[ ocpp16 /Authorize.json] -> null
So a folder with a trailing slash resolves under gradle test and in the IDE, and returns null from the published jar. Green CI, every message rejected in production. schemaFolder.trim().trimEnd('/') in the init block closes it.
Since the constructor signature is breaking anyway, the deeper option is to take a type instead of a String. Something like enum class OcppSchemaFolder(val path: String) { OCPP_1_2("ocpp12"), OCPP_1_5("ocpp15"), OCPP_1_6("ocpp16"), OCPP_2_0("ocpp20") } removes this require, both blank-folder test cases, the whitespace and trailing-slash hazards, and the four hand-copied SCHEMA_FOLDER constants at once, and makes it impossible for a new version module to reuse an existing folder or fall back to the root. (The two existing OcppVersion enums in ocpp-wamp and ocpp-transport are not visible from ocpp-json, so this would want its own type.)
There was a problem hiding this comment.
Took the enum. OcppSchemaFolder (29b19ae) replaces the String: one entry per version, so blank, whitespace and trailing-slash values are unrepresentable and the four SCHEMA_FOLDER constants go away with the require and both blank-folder tests.
| val file = "$schemaFolder/$action.json" | ||
| val factory: JsonSchemaFactory = JsonSchemaFactory.getInstance(specVersion) | ||
| val input: InputStream? = Thread.currentThread().contextClassLoader.getResourceAsStream(file) | ||
| val input: InputStream = checkNotNull(Thread.currentThread().contextClassLoader.getResourceAsStream(file)) { |
There was a problem hiding this comment.
Better than Jackson's argument "in" is null, but this message never reaches a caller going through a parser.
getJsonSchema is reached from OcppJsonParser.parseAnyFromString, whose body is wrapped in a blanket catch (e: Exception). IllegalStateException is not an OcppParserException, so it lands in that catch and comes back as JsonMessage(msgType=CALL_ERROR, errorCode=INTERNAL_ERROR) with the text buried in a stack trace inside errorDetails. Every message for that action gets a generic protocol error and nothing names the missing schema.
OcppJsonValidatorTest.names the missing schema when it is not on the classpath exercises the validator directly and never through a parser, so the suite looks like it covers this when it does not.
Resolving the schemas eagerly at parser construction, or rethrowing as an OcppParserException so it survives the catch, would make a misconfigured folder fail loudly instead of degrading to INTERNAL_ERROR.
There was a problem hiding this comment.
Rethrown as SchemaNotFoundException : OcppParserException (29b19ae), so the parser's catch keeps it and the call error carries ErrorDetail(code="schema", detail="ocpp16/Heartbeat.json"). I did not go for eager loading: WebsocketServer builds a parser per connection, so it would front-load every schema on each one. OcppJsonValidatorTest now also drives a minimal parser subclass end to end and asserts the detail survives the catch; it was red before the change.
| private fun getJsonSchema(action: String): JsonSchema { | ||
| val file = if (schemaFolder.isEmpty()) "$action.json" else "$schemaFolder/$action.json" | ||
| val file = "$schemaFolder/$action.json" | ||
| val factory: JsonSchemaFactory = JsonSchemaFactory.getInstance(specVersion) |
There was a problem hiding this comment.
Not introduced here, but the function is being touched: the factory is rebuilt on every cache miss. A long-lived Ocpp20JsonParser that sees the full action set builds 126 JsonSchemaFactory instances, each re-registering the draft meta-schema. Hoisting it to a private val next to config would make it one.
There was a problem hiding this comment.
Hoisted to a private val next to config (29b19ae).
| val input: InputStream = checkNotNull(Thread.currentThread().contextClassLoader.getResourceAsStream(file)) { | ||
| "Schema $file not found on the classpath" | ||
| } | ||
| return factory.getSchema(input, config) |
There was a problem hiding this comment.
The stream is not closed if getSchema throws (malformed schema file, unsupported $schema draft). Now that checkNotNull guarantees non-null, input.use { factory.getSchema(it, config) } costs nothing.
There was a problem hiding this comment.
Done with input.use { factory.getSchema(it, config) } (29b19ae).
| } | ||
|
|
||
| private companion object { | ||
| const val SCHEMA_FOLDER = "ocpp16" |
There was a problem hiding this comment.
This constant and the resource directory are now the contract the whole fix rests on, and nothing checks that they agree. No test in any of the four json modules iterates Actions.entries / Actions.values().
Rename src/main/resources/ocpp16, or add an Actions entry without its schema, and every module suite stays green; JsonSchemaIsolationTest stays green too, since it only exercises Authorize and BootNotification. The break then shows up in production, for that one action, as the swallowed INTERNAL_ERROR described on OcppJsonValidator.kt.
A per-module test asserting that every Actions entry resolves $SCHEMA_FOLDER/<camelCase>Request.json and ...Response.json would pin it, and would also catch the trailing-slash case.
There was a problem hiding this comment.
Added SchemaResolutionTest in each of the four json modules (bbafc09): @EnumSource(Actions::class), one empty-payload parse per action for request and response, rejecting an INTERNAL_ERROR. Verified red by hiding HeartbeatResponse.json from 1.6. It immediately caught a real gap: 2.0 shipped 126 of the 128 spec schemas, Get15118EVCertificate request and response were missing, so that action failed on dev with validation on. Added in 58ebcc6 with a round-trip test.
| } | ||
|
|
||
| @Test | ||
| fun `accepts a schema folder`() { |
There was a problem hiding this comment.
This one asserts nothing, and "ocpp16" reads as if it were checking a real folder. ocpp-json does not depend on ocpp-1-6-json, and schema loading is lazy, so the constructor never touches the classpath: replacing "ocpp16" with "x" keeps it green.
All it covers is the negation of the require, which the parameterized test above already covers. Either drop it, or use a literal that does not suggest a folder that is not there.
There was a problem hiding this comment.
Dropped. With the enum there is nothing left to assert; the remaining validator test targets SchemaNotFoundException and the missing path.
| /** | ||
| * Every ocpp-*-json module ships schemas under the same action names, and OcppJsonValidator resolves | ||
| * them off the classpath. The collision is only observable where several versions share a classpath, | ||
| * i.e. here in toolkit. Each payload below is valid in its own version but rejected by the schema of |
There was a problem hiding this comment.
"Each payload below is valid in its own version but rejected by the schema of the older version that would otherwise shadow it" holds for two of the four. I applied this file and the toolkit build change onto dev: 4 tests, 2 failures, the 2.0 and 1.6 cases.
The 1.5 case passes on dev because classpath order already favours ocpp-1-5-json, and the 1.2 case cannot fail at all since ocpp12/ was namespaced in #107. Both are still worth keeping as forward-looking guards, but someone reading this doc will take all four for reproductions and may drop one of the new testImplementation lines in toolkit/build.gradle.kts without realising it weakens the guard. Worth saying which two reproduce the collision and which two are guards.
…ema in the call error
A String folder still let a trailing slash or surrounding whitespace through:
"ocpp16/" resolves from an exploded directory (gradle test, IDE) and returns
null from the published jar. Replace it with OcppSchemaFolder, one entry per
version, so the folders are distinct and well-formed by construction, and drop
the blank-folder guard and the four hand-copied constants with it.
A missing schema used to land in OcppJsonParser's blanket catch as a bare
INTERNAL_ERROR with the file name buried in a stack trace. Raise
SchemaNotFoundException, an OcppParserException, so the call error carries the
schema path in its details. Loading stays lazy: parsers are built per
connection and eager loading would front-load every schema on each one.
Also build the JsonSchemaFactory once per validator instead of once per cache
miss, close the schema stream if parsing throws, and drop kotlin("test-junit")
from ocpp-json now that it has tests: the runner is JUnit Platform without a
vintage engine, so JUnit 4 tests would compile and silently never run.
BREAKING CHANGE: OcppJsonValidator takes an OcppSchemaFolder instead of a
String, e.g. OcppJsonValidator(SpecVersion.VersionFlag.V4, OcppSchemaFolder.OCPP_1_6).
The OCPP 2.0.1 part 3 set has 128 schemas; the module shipped 126. With validation enabled every Get15118EVCertificate request and response failed with an internal error. Add the two files from the specification, with the "$id" removed like the others.
Nothing checked that a parser's schema folder and the resources agree: renaming the folder or adding an action without its schema kept every suite green and broke in production for that one action. SchemaResolutionTest parses an empty payload for every Actions entry, request and response, and rejects an INTERNAL_ERROR, which is how a missing schema now surfaces. It is what caught the Get15118EVCertificate gap. Also state in JsonSchemaIsolationTest which two cases reproduce IZIVIA#111 and which two are guards, so nobody drops a toolkit test dependency thinking it is unused.
|
Thanks for the thorough pass. All inline threads answered and addressed in 29b19ae, 58ebcc6 and bbafc09. On the two points without a diff line: |
pbourseau
left a comment
pbourseau
left a comment
There was a problem hiding this comment.
Re-reviewed at bbafc09. All nine points are addressed, and the exhaustive test earned its keep: ocpp-2-0-json shipping 126 of the 128 OCPP 2.0.1 schemas is a production bug in its own right, independent of the classpath collision this PR set out to fix. Nice catch.
Checked on this head: :ocpp-json, the four ocpp-*-json modules, :ocpp-1-6-security and :toolkit are green, 701 tests. OcppSchemaFolder makes the trailing-slash case unrepresentable, the factory is built once, the stream is closed, the JUnit 4 dependency is gone, and SchemaNotFoundException does carry the schema path into the call error — the new parser test proves it.
One thing that path still drops, inline.
| * Raised when a schema is missing from the classpath. Extends [OcppParserException] so that | ||
| * [OcppJsonParser] reports the schema in the returned call error instead of a bare internal error. | ||
| */ | ||
| class SchemaNotFoundException(schema: String) : OcppParserException( |
There was a problem hiding this comment.
This is the only OcppParserException subclass that does not carry messageId.
Every sibling in utils/Exceptions.kt — ValidationException, FormatViolationException, ActionRequestNullOrUnknownException, MessageTypeException, MalformedOcppMessageException, ActionResponseNotSpecifiedException — declares override val messageId: String? and forwards it. This one takes the OcppParserException default of null, and OcppJsonParser.jsonMessage then substitutes "Unknown".
Measured on this head, parsing [2,"REQ-42","Heartbeat",{}] against a missing schema:
sent msgId=REQ-42 | returned msgId=Unknown | errorCode=INTERNAL_ERROR
OCPP-J requires a CALL_ERROR to echo the MessageId of the CALL it answers, so the charging station cannot correlate this one and waits out its timeout instead. It was equally "Unknown" before, through the blanket catch — but routing this through OcppParserException was precisely about making the call error usable, and the id is available at the call site: validateJson has jsonMessage.msgId, so catching and rethrowing with it (or threading it through isValidObject) would finish the job.
reports the missing schema in the call error returned by a parser already sends "messageId" and asserts the details — one more assertion there would pin it.
There was a problem hiding this comment.
Right, that was half a fix. isValidObject now takes the messageId (no default, so a caller cannot drop it) and SchemaNotFoundException forwards it like its siblings (e099434). The parser-level test asserts msgId == "messageId" on the call error; it failed with Unknown before the change.
SchemaNotFoundException was the only OcppParserException without a messageId, so OcppJsonParser answered with msgId "Unknown" and the charging station could not correlate the CALL_ERROR with its CALL. Thread the id through isValidObject, where every parser has it, and pin it in the parser-level test.
|
2 participants
Fix #111
Problem
OcppJsonValidatorresolves schemas as bare classpath resources by action name.ocpp-1-5-json,ocpp-1-6-jsonandocpp-2-0-jsonall shipped theirs at the resources root under overlapping names (57 shared between 1.6 and 2.0, 24 between 1.5 and 1.6). On a shared classpath —toolkit, hence every published consumer —getResourceAsStreamreturns the oldest jar's file, so a valid OCPP 2.0.1Authorizerequest was rejected against the 1.6AuthorizeRequestschema. Per-module suites never saw it because each module only has its own schemas on its classpath.Changes
fix(ocpp-1-5-json,ocpp-1-6-json,ocpp-2-0-json,toolkit): namespace JSON schemas per OCPP versionocpp15/,ocpp16/andocpp20/(the OCA licence notice ofocpp-1-6-jsonandocpp-2-0-jsonfollows them), asocpp-1-2-jsonalready does withocpp12/since feat: add OCPP 1.2 support over JSON/WebSocket #107. Plaingit mv, no schema content changed; none of them uses an external$ref, so the move is safe.Ocpp15JsonParser,Ocpp16JsonParserandOcpp20JsonParserpass their folder toOcppJsonValidator.ocpp-1-6-securityships no schemas of its own; the security-whitepaper ones live inocpp-1-6-jsonand follow it intoocpp16/.JsonSchemaIsolationTestintoolkit, the only place the collision is observable. One payload per version, each valid in its own version but rejected by the older schema that used to shadow it. The 2.0 and 1.6 cases fail ondevbefore the fix.refactor(ocpp-json)!: type the schema folder and report a missing schema in the call errorOcppJsonValidatortakes anOcppSchemaFolderenum, one entry per version, so folders are distinct and well-formed by construction; the four per-parser constants go away.SchemaNotFoundException, anOcppParserException, so the call error carriesErrorDetail(code="schema", detail="<folder>/<Action>.json")instead of a bare internal error. Loading stays lazy: parsers are built per connection.JsonSchemaFactoryis built once per validator, the schema stream is closed if parsing throws, andkotlin("test-junit")is dropped fromocpp-json(JUnit Platform runner, no vintage engine).fix(ocpp-2-0-json): ship theGet15118EVCertificateschemastest: pin every action to a shipped schemaSchemaResolutionTestin each json module parses an empty payload for everyActionsentry, request and response, and rejects anINTERNAL_ERROR.Breaking change
OcppJsonValidator(specVersion)no longer compiles. Pass the folder holding the schemas, e.g.OcppJsonValidator(SpecVersion.VersionFlag.V4, OcppSchemaFolder.OCPP_1_6). The four bundled parsers are already updated; only code that instantiatesOcppJsonValidatordirectly is affected.Verification
:ocpp-json,:ocpp-1-2-json,:ocpp-1-5-json,:ocpp-1-6-json,:ocpp-2-0-json,:ocpp-1-6-security,:ocpp-transport-websocket,:toolkittest suites green (707 tests; the 4 skips intoolkitare the pre-existingExampleTestgated on a local SteVe).