feat: add OCPP 1.2 support and OCPP 1.5 API adapter

[juherr]

Closes #84

Summary

Adds first-class support for OCPP 1.2 (SOAP only, as requested in #84) and completes the OCPP 1.5 API adapter, so applications can talk to 1.2/1.5 charge points and CSMS through the same version-agnostic generic API already used for 1.6 and 2.0. This rounds out the library's version coverage — one of the goals in #84 (positioning it as a base library for a Steve-style OSS CPMS).

Previously ApiFactory threw NotImplementedError("Ocpp 1.5 api adapter not yet implemented") and had no 1.2 path at all. Both are now wired end to end.

Breaking changes

Separating diagnostics from log status (see below) changes two things beyond OCPP 1.2/1.5. Both compile fine and fail at runtime, so they need a deliberate look even if you only use 1.6.

logStatusNotification on OCPP 1.6 now requires the security whitepaper. It used to fall back to the core DiagnosticsStatusNotification when securityExtensions = false; it now throws IllegalStateException in that case, like every other whitepaper operation.

// before — core GetDiagnostics flow, whitepaper off
csmsApi.logStatusNotification(meta, LogStatusNotificationReq(status = Uploaded))

// after
csmsApi.diagnosticsStatusNotification(meta, DiagnosticsStatusNotificationReq(status = Uploaded))

logStatusNotification keeps its meaning where the message actually exists: OCPP 2.0.1, and 1.6 with the whitepaper enabled. On 1.2 and 1.5 it is rejected outright, those versions have no such message.

CSMSApi gains an abstract diagnosticsStatusNotification. The interface has no default method bodies, so out-of-tree implementations must add it. CSMSApiCallbacks gets a defaulted one and is unaffected.

What's included

• OCPP 1.2 SOAP (ocpp-1-2-soap, ocpp-1-2-core, ocpp-1-2-api, ocpp-1-2-api-adapter): SOAP parser, core model and the Ocpp12Adapter / Ocpp12CSApiAdapter translation layer.
• OCPP 1.5 API adapter (ocpp-1-5-api-adapter): Ocpp15Adapter plus the full set of request/response mappers, mirroring the mature 1.6 adapter.
• A generic diagnosticsStatusNotification operation. LogStatusNotification (2.0.1 and the 1.6-J whitepaper) and DiagnosticsStatusNotification (1.2/1.5/1.6) are different messages, but the generic API only exposed the former — so the 1.x adapters routed it onto the diagnostics wire message, and one generic operation meant two things depending on the negotiated version. The missing operation now exists, modelled on 1.6, the richest version to define it. OCPP 2.0.1 rejects it, having replaced the message.
• Integration wiring:
  • OcppVersion enum gains OCPP_1_2("ocpp1.2").
  • ApiFactory / CSMS now build 1.2 and 1.5 adapters (the when over versions is exhaustive).
  • Settings.newMessageId: () -> String (default UUID.randomUUID) makes message-id generation injectable — removes static UUID mocking from tests.
  • OCPP 1.2 over WebSocket is reported explicitly instead of failing on an opaque No enum constant. The toolkit has no OCPP 1.2 JSON module; the protocol itself does allow JSON over WebSocket for 1.2, and a follow-up PR adds the module.

Generic → version mapping: graceful degradation

The generic model (2.0-shaped) is richer than the 1.2/1.5 wire models. Every downgrade conversion is exhaustive and explicit (no Enum.valueOf(name) fallback), so the compiler forces each value to be handled and a new generic value cannot silently crash at runtime. Values with no target equivalent are handled deliberately:

• Error codes with no equivalent → Mode3Error (1.2, which has no OtherError) / OtherError (1.5), with a warn log.
• Connector Reserved (1.2 only) → Unavailable; RebootRequired config status → Accepted; reset Scheduled → Accepted.
• Transient firmware/diagnostics states (Downloading, Installing, Uploading, Idle, …) have no terminal 1.x equivalent → the adapter skips them (isSupported), logs a warn, and returns RequestStatus.NOT_SEND rather than forwarding a misleading value.
• Charging states → Occupied, including EVConnected on a transaction end: 1.x has no Finishing, and announcing Available would offer a connector whose cable is still plugged in.
• Sampled values with no 1.5 equivalent (measurands such as SoC, contexts Trigger/Other, locations Cable/EV) are rejected rather than relabelled, and the MeterValues degrades to NOT_SEND. Units resolve on the OCPP 1.5 wire spelling, with the SI symbols aliased so an ampere reading is not reported as Wh.

Where OCPP 1.2 is genuinely more permissive than its successors, the adapter follows the spec rather than the 1.6 code it derives from: a BootNotification response may legally omit currentTime and heartbeatInterval, so a bare Rejected is mapped instead of raising.

Testing

• Adapter/mapper unit tests for 1.2 and 1.5: round-trip mapping, enum downgrade over every value of the generic enums, transient-state filtering, unsupported-operation rejection, repository lifecycle.
• Toolkit factory tests split per version (Ocpp12FactoryTest, Ocpp15FactoryTest) over a shared SOAP helper, plus the WebSocket subprotocol negotiated per version.
• generic-api gains its first tests, covering the send() dispatch and the default CSMSApi implementation.
• ./gradlew build → green.

[pbourseau]

Revue de la PR #98 (OCPP 1.2 + adaptateur 1.5). Belle contribution, cohérente avec l'adaptateur 1.6 existant : when exhaustifs sur les enums, dégradations explicites et loguées, tests par version. Quelques points ci-dessous en commentaires en ligne — aucun n'est bloquant, mais #1 (StopTransaction qui lève une exception si l'état est perdu) et #2 (filtrage MeterValues incohérent) méritent d'être traités ou explicitement assumés avant merge.

[pbourseau]

Robustesse — StopTransaction lève une exception si l'état est perdu. Contrairement à meterValues (qui attrape l'IllegalStateException et renvoie NOT_SEND), ce lookup n'est pas protégé. getTransactionIdsByLocalId lève IllegalStateException("key … not found") : un StopTransaction dont le Start n'a pas été enregistré — typiquement après un redémarrage, puisque RealTransactionRepository est en mémoire — fait remonter l'exception.

Soit attraper l'exception de façon cohérente (comme dans meterValues), soit documenter que le repository par défaut n'est pas persistant et que l'appelant doit injecter sa propre implémentation.

[juherr]

Documenté plutôt qu'attrapé : RealTransactionRepository porte désormais un KDoc précisant qu'il est en mémoire / non persistant et qu'il faut injecter une implémentation persistante pour survivre à un redémarrage. À la différence de meterValues, avaler un StopTransaction (→ NOT_SEND) risquerait de perdre silencieusement une fin de session/facturation. (d55edde)

[pbourseau]

Effet de bord — mutation de l'argument d'entrée. request.timestamp est muté en place, alors que la même instance request est ensuite renvoyée dans l'OperationExecution. Muter un argument d'entrée est un smell ; préférer une copie (request.copy(timestamp = …)).

[juherr]

Corrigé : on copie la requête (request.copy(timestamp = …)) au lieu de muter l'argument, dans updateStatusEvent (1.2 et 1.5). (d55edde)

[pbourseau]

Cohérence — deux règles de filtrage MeterValues divergentes. Ici : singleOrNull { measurand == EnergyActiveImportRegister }, sans filtre de contexte, et on exige exactement un. À l'inverse CommonMapper.filterMeterValues (utilisé par Start/Stop) filtre par contexte + measurand et tolère « au plus un ».

Pour un même concept (EnergyActiveImportRegister), une même entrée peut être acceptée sur un chemin et rejetée sur l'autre. À unifier.

[juherr]

Constat juste. Je préfère ne pas unifier dans cette PR : le chemin MeterValues n'a pas de ReadingContext fixe (contrairement à Start/Stop qui filtrent sur Transaction.Begin/End), donc aligner sur filterMeterValues changerait les valeurs acceptées/rejetées et mérite son propre changement avec des tests dédiés. À traiter en suivi.

[juherr]

Unifié (option A) : la sélection de la valeur EnergyActiveImportRegister est extraite dans CommonMapper.singleEnergyRegister(sampledValues, context?). Le chemin MeterValues l'appelle sans contexte, Start/Stop (filterMeterValues) avec TransactionBegin/End ; la règle mesurande + cardinalité est désormais commune, donc un même jeu de sampled values est accepté/rejeté de façon cohérente. Pas de changement de comportement, + un test dédié (0/1/n valeurs, avec et sans contexte). (45988b1)

[pbourseau]

Spec — remoteStartId peut être négatif. Random.nextInt() couvre tout l'intervalle Int, y compris les valeurs négatives, alors que remoteStartId OCPP est attendu positif. Utiliser p.ex. Random.nextInt(1, Int.MAX_VALUE).

[juherr]

Corrigé : Random.nextInt(1, Int.MAX_VALUE). (d55edde)

[pbourseau]

Encapsulation. hashMap est public : il expose l'état mutable interne. Le passer en private.

[juherr]

Corrigé : hashMap passé en private. (d55edde)

[pbourseau]

Nommage (copier-coller). La fonction s'appelle convertFirmwareStatus alors qu'elle convertit un statut diagnostics. Le KDoc de unsupportedStatuses plus haut mentionne aussi « convertFirmwareStatus ». À renommer en convertDiagnosticsStatus pour la clarté.

[juherr]

Corrigé : renommé convertDiagnosticsStatus (+ KDoc de unsupportedStatuses mis à jour), 1.2 et 1.5. (d55edde)

[pbourseau]

TODO() résiduel. Ce when retombe sur TODO("Not yet implemented") pour OCPP_2_0, qui lève NotImplementedError à l'exécution — alors que la PR met en avant l'exhaustivité. Latent (2.0 est websocket-only), mais un IllegalArgumentException("OCPP 2.0 has no SOAP transport") explicite serait plus clair qu'un TODO.

[juherr]

Corrigé : getSoapParser gère explicitement OCPP_2_0 -> throw IllegalArgumentException("OCPP 2.0 has no SOAP transport") ; le when est désormais exhaustif (plus de else/TODO). (d55edde)

[pbourseau]

Robustesse (idem 1.2). Même remarque que pour Ocpp12Adapter : ce lookup non protégé lève une IllegalStateException si le Start n'a pas été enregistré (repository en mémoire, perdu au redémarrage). À attraper ou à documenter.

[juherr]

Idem 1.2 : documenté sur RealTransactionRepository (1.5). (d55edde)

[pbourseau]

Spec (idem 1.2). Random.nextInt() peut produire un remoteStartId négatif ; utiliser Random.nextInt(1, Int.MAX_VALUE).

[juherr]

Corrigé (idem 1.2) : Random.nextInt(1, Int.MAX_VALUE). (d55edde)

[pbourseau]

Encapsulation (idem 1.2). hashMap devrait être private.

[juherr]

Corrigé (idem 1.2) : hashMap en private. (d55edde)

[sonarqubecloud]

Quality Gate passed

Issues
13 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
24.3% Duplication on New Code

See analysis details on SonarQube Cloud

[juherr]

Ping @pbourseau

[pbourseau]

Merci pour les corrections du premier tour : les 11 remarques inline ont toutes été traitées ou explicitement assumées avec justification (repository documenté plutôt qu'attrapé, copy() au lieu de la mutation, unification singleEnergyRegister, getSoapParser exhaustif...). Rien à redire sur cette boucle.

Cette seconde passe a creusé les mappers 1.5, le routage des actions et le cycle de vie du TransactionRepository. Elle remonte des points nouveaux, dont deux bloquants.

Bloquant

1. Quatre Enum.valueOf qui lèvent sur des données OCPP valides (commentaires sur adapter15/mapper/CommonMapper.kt).

La description affirme :

Every enum conversion is now exhaustive and explicit (no Enum.valueOf(name) fallback), so the compiler forces each value to be handled and a new generic value can't silently crash at runtime.

Ce n'est pas le cas : il reste 12 valueOf dans les nouveaux adaptateurs. Huit sont sûres aujourd'hui (j'ai comparé les enums deux à deux), mais quatre crashent sur des valeurs parfaitement légales — mesurande SoC, contexte Trigger, emplacement Cable, unité var. Elles sont toutes sur convertSampledValue, donc sur le chemin de chaque MeterValues / StartTransaction / StopTransaction 1.5.

Aggravant : Enum.valueOf lève IllegalArgumentException, alors que le catch de meterValues ne vise que IllegalStateException. Ces quatre cas remontent donc à l'appelant au lieu de la dégradation NOT_SEND annoncée. Et MapperTest ne couvre que EnergyActiveImportRegister avec contexte et unité par défaut, donc la CI ne voit rien.

Le convertErrorCode de adapter15/mapper/StatusNotificationMapper.kt est exactement le bon modèle : when exhaustif, valeurs sans équivalent regroupées dans une branche commentée avec warn. C'est ce qui manque à CommonMapper.

2. @Throws et catch désalignés sur MeterValues 1.2 (commentaire sur adapter12/mapper/MeterValuesMapper.kt).

À traiter dans la foulée

• OCPP 1.2 + WebSocket produit un No enum constant com.izivia.ocpp.OcppVersion.OCPP_1_2 illisible, alors que le cas symétrique (2.0 + SOAP) a reçu un message clair dans cette même PR.
• TransactionRepository n'a aucune opération de suppression → la map grossit d'une entrée par transaction, pour la durée de vie du process.
• TransactionEvent Updated sans chargingState lève une exception non attrapée.

À arbitrer

• Settings.newMessageId entre en collision directe avec la #97, qui ajoute le même paramètre sur ApiFactory.getCSMSApi(...) pour le même besoin. Une seule des deux PR doit le porter.
• logStatusNotification : choix inverse de celui de la #97. Ici 1.2/1.5 mappent vers DiagnosticsStatusNotification ; la #97 reroute la même opération générique vers le LogStatusNotification du whitepaper sécurité en 1.6. Si les deux PR mergent en l'état, l'opération générique n'aura pas la même sémantique selon la version — précisément ce que la couche d'adaptation est censée éviter. À trancher entre les deux PR.
• Complétude de core12.ChargePointErrorCode (commentaire dédié).
• 24,3 % de duplication et 13 nouvelles issues SonarCloud non triées. La duplication est attendue (1.5 duplique 1.6, 1.2 duplique 1.5), mais ça fait maintenant 3 copies des mêmes mappers : une base commune mérite d'être envisagée avant qu'une 4e n'arrive.

Ce qui est solide

La couverture de tests est réelle et pertinente (AdapterTest 393 l., CSApiAdapterTest 381 l. côté 1.5, plus les équivalents 1.2 et les factory tests par version). Le filtrage isSupported des statuts transitoires avec warn + NOT_SEND est la bonne dégradation. Les dégradations documentées le sont vraiment (Reserved -> Unavailable, RebootRequired -> Accepted, Charging/SuspendedEV/SuspendedEVSE -> Occupied). Et l'unification singleEnergyRegister faite en cours de revue est propre et testée.

C'est une contribution ambitieuse qui complète réellement la couverture de versions visée par #84 — les points ci-dessus sont ciblés, pas une remise en cause de l'approche.

[pbourseau]

Crash sur Trigger et Other.

ReadingContextEnumType (générique) a 8 valeurs, core15.ReadingContext seulement 6 :

Générique	core15
InterruptionBegin, InterruptionEnd, SampleClock, SamplePeriodic, TransactionBegin, TransactionEnd	idem
Trigger	absent
Other	absent

java.lang.IllegalArgumentException: No enum constant com.izivia.ocpp.core15.model.common.enumeration.ReadingContext.Trigger

Trigger et Other sont des contextes standards en OCPP 1.6 et 2.0, couramment émis. Et comme c'est une IllegalArgumentException, le catch (e: IllegalStateException) de Ocpp15Adapter.meterValues ne la rattrape pas : elle remonte à l'appelant au lieu de dégrader en NOT_SEND.

À convertir en when exhaustif, avec les deux valeurs sans équivalent regroupées dans une branche commentée + warn, comme le fait déjà StatusNotificationMapper.convertErrorCode.

[juherr]

Corrigé (6b4c4ba9).

convertReadingContext est désormais un when exhaustif sans else. Trigger et Other sont rejetés explicitement plutôt que rabattus : les six contextes de core15 sont tous des Sample.* / Transaction.* / Interruption.*, aucun ne décrit une lecture déclenchée à la demande, et choisir Sample.Clock reviendrait à mal étiqueter la mesure.

INVALID REQUEST : ReadingContext.Trigger doesn't exists in OCPP 1.5

Sur le point de l'exception : meterValues attrape maintenant aussi IllegalArgumentException, donc ces cas dégradent bien en NOT_SEND (voir le fil sur Ocpp12Adapter:118).

Test : CommonMapperConversionTest itère sur toutes les valeurs de ReadingContextEnumType et vérifie que chacune donne soit la cible 1.5 attendue, soit un rejet explicite — jamais un No enum constant.

[pbourseau]

Crash sur Cable et EV.

LocationEnumType (générique) : Cable, EV, Inlet, Outlet, Body.
core15.Location : Inlet, Outlet, Body seulement.

java.lang.IllegalArgumentException: No enum constant com.izivia.ocpp.core15.model.common.enumeration.Location.Cable

Même remarque que pour convertReadingContext juste au-dessus : when exhaustif, et choisir explicitement la dégradation (Cable/EV -> Outlet ? avec warn).

[juherr]

Corrigé (6b4c4ba9), when exhaustif sans else.

J'ai retenu le rejet plutôt que Cable/EV → Outlet : Location décrit où la mesure a été prise, et annoncer en sortie de borne une mesure faite dans le câble ou dans le véhicule fabrique une donnée fausse — exactement ce qu'on a voulu éviter ailleurs. Une valeur absente vaut mieux qu'une valeur trompeuse.

Test : CommonMapperConversionTest balaie les 5 valeurs de LocationEnumType.

[pbourseau]

Le else rattrape six valeurs qui n'existent pas en 1.5.

Cinq mesurandes sont explicitement gardées juste au-dessus (EnergyActiveNet, EnergyReactiveNet, EnergyApparent*), mais le else laisse passer :

PowerOffered, PowerFactor, CurrentOffered, Frequency, SoC, RPM

Aucune n'existe dans core15.Measurand :

java.lang.IllegalArgumentException: No enum constant com.izivia.ocpp.core15.model.common.enumeration.Measurand.SoC

SoC (état de charge batterie) est une des mesures les plus courantes envoyées par une borne DC — c'est probablement le crash le plus probable en production des quatre remontés dans ce fichier.

Incohérence supplémentaire dans cette même fonction : les cinq valeurs gardées lèvent IllegalStateException (donc attrapée par meterValues -> NOT_SEND), les six autres IllegalArgumentException (non attrapée -> crash). Deux comportements opposés pour le même type de problème.

En remplaçant le else par les six valeurs explicites, le compilateur garantira la couverture et la prochaine valeur ajoutée au modèle générique cassera la build au lieu de la prod.

[juherr]

Corrigé (6b4c4ba9). Le else a disparu : les 16 mesurandes de core15 sont mappés un à un, et les 11 sans équivalent — les 5 déjà gardés plus PowerOffered, PowerFactor, CurrentOffered, Frequency, SoC, RPM — partagent une branche unique. Le compilateur garantit maintenant la couverture.

L'incohérence des deux exceptions est levée dans le même mouvement : toute erreur de mapping lève désormais IllegalArgumentException, convention alignée sur CommonMapper.filterMeterValues en 1.6 et singleEnergyRegister en 1.2, et les catch des adapters suivent. Les 11 valeurs dégradent donc uniformément en NOT_SEND.

Test : CommonMapperConversionTest itère sur les 27 valeurs de MeasurandEnumType.

[pbourseau]

Le garde ne teste pas ce que valueOf résout.

La condition ligne 80 compare le value de l'enum, valueOf ligne 81 résout par name. Or core15.UnitOfMeasure contient une entrée où les deux diffèrent :

Var("var"),   // name = "Var", value = "var"

Donc pour unitOfMeasure.unit == "var" : le garde passe (it.value == "var" OK), puis

java.lang.IllegalArgumentException: No enum constant com.izivia.ocpp.core15.model.common.enumeration.UnitOfMeasure.var

Proposition :

private fun convertUnit(value: UnitOfMeasureGen?): UnitOfMeasure =
    enumValues<UnitOfMeasure>().firstOrNull { it.value == value?.unit } ?: UnitOfMeasure.Wh

Un seul parcours, plus de désaccord name/value possible.

Effet de bord à noter au passage : le repli ?: UnitOfMeasure.Wh s'applique à toute unité inconnue. En OCPP 2.0, UnitOfMeasureType.unit est une chaîne libre (max 20 car.) et les unités canoniques sont "A" / "V" / "W", pas "Amp" / "Volt". Une mesure de courant en ampères est donc silencieusement réétiquetée Wh. Un warn est le minimum ; un mapping explicite "A" -> Amp, "V" -> Volt serait mieux.

[juherr]

Corrigé (6b4c4ba9), en reprenant ta proposition : un seul parcours sur .value, plus aucun désaccord name/value possible.

J'ai aussi traité l'effet de bord que tu signales. OCPP 1.5 écrit Amp/Volt là où le modèle générique utilise les symboles SI, donc un alias explicite "A" -> Amp, "V" -> Volt ; et le repli Wh émet désormais un logger.warn.

J'ai gardé le repli plutôt que de rejeter : UnitOfMeasureType.unit est une chaîne libre (20 car.) en 2.0, donc rejeter toute valeur inconnue serait plus dur que le comportement actuel et casserait des MeterValues aujourd'hui acceptés. Dis-moi si tu préfères le rejet.

Test : CommonMapperConversionTest couvre "var" (qui plantait), "Var" (qui retombait silencieusement sur Wh), "A", "V", "Wh", une unité inconnue et l'absence d'unité.

[pbourseau]

@Throws mensonger, et l'exception n'est pas attrapée.

L'annotation déclare IllegalStateException, mais le code lève IllegalArgumentException dans les deux chemins d'erreur :

• ligne 21, quand aucun EnergyActiveImportRegister n'est présent ;
• dans CommonMapper.singleEnergyRegister, quand il y en a plusieurs (At most 1 EnergyActiveImportRegister sampled value expected).

Et Ocpp12Adapter.meterValues n'attrape que IllegalStateException. Les deux cas remontent donc non gérés, alors que toute la structure try/catch de la méthode a été écrite pour les dégrader en NOT_SEND.

Même famille de problème que les quatre valueOf de adapter15/mapper/CommonMapper.kt. Il faudrait choisir une convention d'exception pour les erreurs de mapping et l'appliquer aux deux modules — IllegalArgumentException me semble le bon choix sémantique, à condition d'aligner les catch en conséquence.

[juherr]

Corrigé (8b073259). L'annotation devient @Throws(IllegalArgumentException::class), qui est bien ce que lèvent les deux chemins.

J'ai suivi ta convention : IllegalArgumentException pour toute erreur de mapping — c'était déjà le choix de filterMeterValues en 1.6 et de singleEnergyRegister en 1.2 — et j'ai aligné les catch en conséquence, en 1.2 comme en 1.5. C'est ce qui rend aussi rattrapables les quatre cas de adapter15/mapper/CommonMapper.kt.

Tests (1.2) : un MeterValues sans EnergyActiveImportRegister et un autre avec plusieurs donnent NOT_SEND, avec verify(exactly = 0) que rien n'est transmis. Les deux échouaient avant le correctif.

[pbourseau]

Aucune opération de suppression : fuite mémoire non bornée.

startTransactionEvent écrit dans la map, stopTransactionEvent lit — et ne nettoie jamais. La ConcurrentHashMap de RealTransactionRepository grossit d'une entrée par transaction, pour toute la durée de vie du process. Sur une borne ou un CSMS qui tourne des mois, la fuite est garantie.

Elle dégrade aussi getLocalIdByTransactionId, déjà en O(n) avec copie (hashMap.toList().find { ... }) — point que tu as assumé à juste titre au premier tour « aux volumes attendus », mais dont le coût croît ici sans borne puisque rien ne sort jamais de la map.

Suggestion : ajouter fun deleteTransactionIds(localId: String) à l'interface et l'appeler dans stopTransactionEvent après un StopTransaction réussi. Concerne 1.2 et 1.5.

Petite imprécision au passage dans le KDoc ajouté sur RealTransactionRepository : « A StopTransaction/MeterValues whose StartTransaction was not recorded will fail the local-id lookup » — exact pour StopTransaction, mais MeterValues dégrade en NOT_SEND (l'exception y est attrapée).

[juherr]

Corrigé (8b073259). TransactionRepository gagne deleteTransactionIds(localId), appelée dans stopTransactionEvent après un StopTransaction réussi, en 1.2 et 1.5. Ça borne la map et, du même coup, le coût du getLocalIdByTransactionId en O(n) que j'avais assumé au premier tour.

J'ai laissé 1.6 de côté : son TransactionRepository est public et pré-existant, donc ajouter une méthode à l'interface casserait les implémentations tierces. À traiter dans un changement dédié si tu veux l'aligner.

Le KDoc est corrigé sur l'imprécision que tu relèves : seul StopTransaction échoue sur un lookup manquant, MeterValues dégrade en NOT_SEND. Il mentionne aussi que les entrées sont libérées à l'arrêt de la transaction.

Test : après un cycle start/stop, getTransactionIdsByLocalId échoue sur l'id local — donc l'entrée a bien été libérée.

[pbourseau]

OCPP 1.2 + WebSocket produit une erreur incompréhensible.

com.izivia.ocpp.transport.OcppVersion gagne bien OCPP_1_2("ocpp1.2") dans cette PR, mais com.izivia.ocpp.OcppVersion (module ocpp-wamp) n'est pas touché :

// ocpp-wamp/src/main/kotlin/com/izivia/ocpp/Ocpp.kt
enum class OcppVersion(val subprotocol: String) {
    OCPP_1_5("ocpp1.5"), OCPP_1_6("ocpp1.6"), OCPP_2_0("ocpp2.0.1")
}

Le valueOf(ocppVersion.name) de cette ligne n'est pas gardé, donc Settings(OCPP_1_2, TransportEnum.WEBSOCKET, ...) donne :

java.lang.IllegalArgumentException: No enum constant com.izivia.ocpp.OcppVersion.OCPP_1_2

Même chose côté serveur : WebsocketServer fait ocppVersions.map { OcppVersionWamp.valueOf(it.name) }, donc un ServerSetting(ocppVersion = setOf(OCPP_1_2), transportType = WEBSOCKET) casse à la construction.

Que 1.2 soit SOAP-only est assumé (#84), pas de souci — mais la PR a justement pris soin de rendre le cas symétrique explicite ligne 276 :

OcppVersionTransport.OCPP_2_0 -> throw IllegalArgumentException("OCPP 2.0 has no SOAP transport")

Faire pareil dans l'autre sens rendrait le message actionnable :

OcppVersionTransport.OCPP_1_2 -> throw IllegalArgumentException("OCPP 1.2 has no WebSocket transport")

[juherr]

Corrigé (9ec8ad22), exactement dans le sens que tu proposes.

Un helper getWampVersion fait maintenant pendant à getSoapParser : when exhaustif sur OcppVersionTransport, avec OCPP_1_2 -> throw IllegalArgumentException("OCPP 1.2 has no WebSocket transport"). Il est utilisé côté client et côté serveur — pour ce dernier, chaque version du Set est validée avant de construire le WebsocketServer, pour que l'échec vienne de la factory avec un message lisible plutôt que du valueOf interne.

ocpp-wamp n'est pas touché : 1.2 reste SOAP-only, comme demandé dans #84.

Tests dans Ocpp12FactoryTest : Settings(OCPP_1_2, WEBSOCKET, ...) et un ServerSetting équivalent lèvent tous deux le message explicite.

[pbourseau]

TransactionEvent Updated sans chargingState lève une exception non attrapée.

updateStatusEvent protège bien l'appel (if (request.transactionInfo.chargingState != null)), mais transactionEvent(TransactionEventEnumType.Updated) appelle updateTransactionEvent directement, sans cette garde.

Un TransactionEvent de type Updated sans chargingState — parfaitement valide en OCPP 2.0, où le champ est optionnel — lève donc cette IllegalArgumentException jusqu'à l'appelant.

Deux options cohérentes avec le reste de la PR : dégrader en NOT_SEND avec un warn, comme pour les statuts transitoires filtrés par isSupported ; ou garder le throw mais le documenter comme un prérequis de l'API générique côté 1.2/1.5.

Même remarque pour l'équivalent 1.5.

[juherr]

Corrigé (8b073259) en 1.2 et 1.5, avec la première option : dégradation en NOT_SEND + warn.

J'ai déplacé la garde dans updateTransactionEvent lui-même plutôt que de la dupliquer chez ses appelants, donc le chemin Updated est couvert comme les chemins Started/Ended. Ça reste cohérent avec le reste de la PR : un événement qu'on ne peut pas représenter sur le fil 1.x est ignoré avec une trace, pas transformé en donnée fausse ni en exception pour l'appelant.

Test : transactionEvent(Updated) sans chargingState renvoie NOT_SEND, avec verify(exactly = 0) sur statusNotification.

[pbourseau]

Question : cette liste est-elle complète au regard de la spec 1.2 ?

8 valeurs ici. core15.ChargePointErrorCode, dans ce même repo, contient exactement les mêmes plus GroundFailure, OverCurrentFailure, UnderVoltage, WeakSignal et OtherError. Un delta de 5 valeurs entre 1.2 et 1.5 me paraît beaucoup pour une révision mineure de la spec.

À ma connaissance, GroundFailure, OverCurrentFailure, UnderVoltage et WeakSignal existent déjà en OCPP 1.2, et 1.5 n'a réellement ajouté que OtherError — mais je ne peux pas le vérifier depuis le repo : il ne contient ni WSDL ni XSD 1.2. C'est donc une question, pas un constat.

Si c'est confirmé, ce n'est pas un problème de mapping mais un modèle core12 incomplet, et la conséquence est dans StatusNotificationMapper.convertErrorCode (lignes 57-68) : le repli vers Mode3Error masquerait une perte d'information évitable, et transformerait notamment un défaut d'isolement (GroundFailure, critique sécurité) en « erreur Mode 3 ».

À trancher contre la spec 1.2.

[juherr]

Bonne question, j'ai vérifié contre la spec : core12 est complet, pas de changement.

Le WSDL officiel OCPP 1.2 (CentralSystemService.wsdl, simpleType ChargePointErrorCode) énumère exactement ces 8 valeurs :

ConnectorLockFailure, HighTemperature, Mode3Error, NoError,
PowerMeterFailure, PowerSwitchFailure, ReaderFailure, ResetFailure

Le WSDL 1.5 (OCPP_CentralSystemService_1.5_FINAL.wsdl) en énumère 13 : les 8 ci-dessus plus GroundFailure, OverCurrentFailure, UnderVoltage, WeakSignal et OtherError. Le delta de 5 est donc bien un ajout de la 1.5, pas une omission du modèle — et 1.5 n'a pas seulement ajouté OtherError.

Ta remarque sur la conséquence reste juste : rabattre GroundFailure sur Mode3Error transforme un défaut d'isolement en erreur Mode 3. C'est une perte réelle, mais 1.2 n'offre aucune cible plus proche ni de catch-all, donc le repli reste, avec son warn.

[juherr]

BTW, you can find specs at https://github.com/juherr/awesome-ev-charging/tree/main/ocpp

[pbourseau]

Collision directe avec la #97.

La #97 ajoute le même paramètre, au même moment, pour le même besoin (supprimer mockkStatic(UUID::class) des tests) — mais sur ApiFactory.getCSMSApi(...) plutôt que sur Settings :

fun getCSMSApi(
    settings: Settings, ocppId: String, csApi: CSApi,
    headers: RequestHeaders = emptyList(),
    newMessageId: () -> String = { UUID.randomUUID().toString() }   // #97
): CSMSApi

Les deux approches se tiennent — le paramètre sur Settings est à mon avis le bon endroit, puisqu'il descend naturellement jusqu'au transport sans élargir la signature publique de la factory. Mais il faut choisir avant de merger l'une des deux, sinon on récupère les deux mécanismes en parallèle.

À noter aussi : Settings est une data class publique, donc l'insertion de ce paramètre avant clientPath décale componentN() et le copy(...) positionnel. Ajout en fin de liste ?

[juherr]

Deux points distincts.

Le décalage componentN() — pas de souci de ce côté : newMessageId est déjà le dernier paramètre de Settings, ajouté après clientPath. Aucun copy(...) positionnel ni componentN() existant n'est décalé.

La collision avec #97 — tranchée dans le sens que tu recommandes, et j'ai fait le pas de plus : cette branche est maintenant rebasée sur ocpp-16-security-messages, et le paramètre que #97 ajoutait sur getCSMSApi est supprimé au profit de settings.newMessageId (d2de739c).

La raison est concrète : celui de #97 ne descend que dans createClientTransportWebsocket, alors que 1.2/1.5 sont SOAP-only et que le chemin serveur en a besoin aussi. Settings le porte déjà jusqu'aux deux transports sans élargir la signature publique de la factory.

Conséquence : #98 est empilée sur #97 et doit être fusionnée après elle. Je ne peux pas re-cibler la base de la PR sur ocpp-16-security-messages (GitHub n'accepte que des branches du dépôt de base, et celle-ci n'existe que sur mon fork), donc le diff affiche temporairement les 4 commits de #97.

[juherr]

Merci pour cette seconde passe — les deux bloquants étaient réels, et le second m'a fait remonter à une cause plus profonde que le symptôme signalé. Tout est traité, chaque correction est couverte par un test, et j'ai répondu sur les 11 fils.

Les deux bloquants

Les quatre valueOf de adapter15/mapper/CommonMapper.kt sont supprimés (6b4c4ba9). Les quatre conversions sont désormais des when exhaustifs sans else, sur le modèle de convertErrorCode que tu citais. Tu avais raison sur la portée : la description de la PR était fausse, ces conversions étaient bien sur le chemin de chaque MeterValues / StartTransaction / StopTransaction 1.5.

Sur l'arbitrage rejet vs repli, j'ai choisi le rejet explicite pour Trigger/Other, Cable/EV et les 11 mesurandes : contrairement à Reserved -> Unavailable ou Charging -> Occupied, il n'existe pas ici de cible sémantiquement juste, et rabattre fabriquerait une donnée fausse (une mesure prise dans le véhicule annoncée en sortie de borne, un SoC annoncé comme une énergie). convertUnit fait exception et garde son repli Wh, mais avec un warn et les alias "A" -> Amp / "V" -> Volt que tu suggérais, parce que unit est une chaîne libre en 2.0 et que rejeter l'inconnu serait plus dur que le comportement actuel.

Le désalignement @Throws / catch est corrigé (8b073259), avec la convention que tu proposes : IllegalArgumentException pour toute erreur de mapping — c'était déjà le choix de filterMeterValues en 1.6 et de singleEnergyRegister en 1.2 — et les catch alignés en 1.2 et 1.5. C'est ce qui rend rattrapables les quatre cas ci-dessus.

Les points « à traiter dans la foulée »

Les trois sont corrigés : getWampVersion rend OCPP 1.2 + WebSocket explicite et symétrique de getSoapParser (9ec8ad22) ; TransactionRepository gagne deleteTransactionIds, appelée au StopTransaction (8b073259) ; et transactionEvent(Updated) sans chargingState dégrade en NOT_SEND + warn au lieu de lever.

Au passage, ton commentaire sur le try trop large valait plus que sa formulation : la garde ne couvre plus l'envoi, donc une panne de transport remonte à l'appelant au lieu d'être maquillée en NOT_SEND.

Les points « à arbitrer »

Settings.newMessageId vs #97. Tranché dans ton sens, et un cran plus loin : cette branche est rebasée sur ocpp-16-security-messages et le paramètre que #97 ajoutait sur getCSMSApi est supprimé au profit de settings.newMessageId (d2de739c). Celui de #97 ne descend que dans le client WebSocket, alors que 1.2/1.5 sont SOAP-only et que le chemin serveur en a besoin aussi.

Conséquence à noter : #98 est maintenant empilée sur #97 et doit être fusionnée après elle. Je ne peux pas re-cibler la base de la PR sur ocpp-16-security-messages, GitHub n'acceptant que des branches du dépôt de base — donc le diff affiche temporairement les 4 commits de #97 (b8d8b082 à 3eb93f36). Si quelqu'un pousse cette branche sur IZIVIA/ocpp-toolkit, je re-cible immédiatement.

logStatusNotification. Ton objection m'a fait creuser, et le problème n'était pas dans le choix de la 1.2/1.5 : CSMSApi n'avait tout simplement pas d'opération diagnosticsStatusNotification. logStatusNotification en était l'unique point d'entrée, d'où le détour des adapters 1.x vers le message diagnostics — et c'est le comportement de dev en 1.6 aujourd'hui, que 1.2/1.5 ne faisaient que reproduire.

Une fois #97 mergée, ce détour rend DiagnosticsStatusNotification inatteignable pour 1.2, 1.5 et 1.6, alors que le message existe dans les trois specs. J'ai donc ajouté l'opération générique manquante (2afbe57c) :

• diagnosticsStatusNotification sur CSMSApi, avec son Req/Resp et son enum, calqués sur 1.6, la version la plus riche à définir ce message ;
• 1.2/1.5 continuent de filtrer les états transitoires Idle/Uploading (warn + NOT_SEND), 1.6 mappe les quatre un à un, 2.0 rejette puisque 2.0.1 a remplacé le message par LogStatusNotification ;
• logStatusNotification est désormais rejetée en 1.2/1.5, qui n'ont pas ce message.

La sémantique de chaque opération générique est donc la même quelle que soit la version négociée, ce que la couche d'adaptation doit garantir. Attention pour les implémentations tierces : CSMSApi n'a pas de corps par défaut, donc cet ajout est source-breaking hors du dépôt.

core12.ChargePointErrorCode. Vérifié contre la spec, le modèle est complet : le WSDL officiel 1.2 énumère exactement ces 8 valeurs, le WSDL 1.5 en énumère 13. Les 5 manquantes — GroundFailure, OverCurrentFailure, UnderVoltage, WeakSignal et OtherError — sont bien des ajouts 1.5. Détail dans le fil dédié.

SonarCloud. Les familles restantes sont le @Mapper abstract class (convention uniforme du projet, à traiter par une exclusion côté config plutôt qu'un refactor) et les casts SOAP non vérifiables (identiques aux parsers 1.5/1.6). Les 6 issues actionnables ont été corrigées en b9c12a0a. La duplication reste, et ta remarque sur la troisième copie des mappers est juste — mais une base commune touche les quatre adapters et mérite sa propre PR.

Vérification

./gradlew build et la suite de tests complète sont verts. Nouveaux tests : CommonMapperConversionTest (1.5) balaie toutes les valeurs de ReadingContextEnumType, LocationEnumType et MeasurandEnumType, plus les cas d'unité ; côté adapters, MeterValues avec SoC, sans / avec plusieurs EnergyActiveImportRegister, panne de transport, libération du repository au StopTransaction, Updated sans chargingState, diagnosticsStatusNotification sur les quatre versions, et OCPP 1.2 + WebSocket dans Ocpp12FactoryTest.

Un seul point reste ouvert de mon côté : le repli Wh de convertUnit, que j'ai gardé plutôt que de rejeter. Dis-moi si tu préfères l'inverse.

[juherr]

Mise à jour depuis le commentaire précédent : la branche a été rebasée sur la
dernière version de #97, ce qui a demandé un arbitrage sur du code venant de cette
PR, et un audit de couverture a comblé quatre trous.

Rebase sur #97, et un arbitrage à valider

#97 a gagné trois commits, dont feat(adapter16): gate the OCPP 1.6 security operations behind a setting. Le rebase a produit quatre conflits ; trois étaient
mécaniques (ApiFactory, Settings, IntegrationTest). Le quatrième mérite une
relecture, parce que git l'a fusionné textuellement sans que ça compile.

Dans sa nouvelle version, Ocpp16Adapter.logStatusNotification choisit entre le
message whitepaper et DiagnosticsStatusNotification selon le réglage, avec ce
commentaire :

The generic API has a single log status operation, so it has to serve both 1.6
flows

Cette prémisse n'est plus vraie : c'est exactement ce que supprime l'ajout de
l'opération générique diagnosticsStatusNotification. Le merge laissait donc la
branche else appeler le mapper diagnostics avec un LogStatusNotificationReq.

J'ai tranché en réutilisant le mécanisme de #97 lui-même :
logStatusNotification appelle désormais checkSecurityExtensions(...) comme
toutes les autres opérations whitepaper, et le flux GetDiagnostics passe par
diagnosticsStatusNotification. L'appelant désigne le flux au lieu que l'adapter
le devine depuis une capacité de station — l'heuristique que le commentaire
décrivait comme impossible à trancher (le requestId ne peut pas arbitrer)
disparaît donc.

Changement de comportement à valider : en 1.6, logStatusNotification sans les
extensions de sécurité lève maintenant, au lieu d'envoyer silencieusement un
DiagnosticsStatusNotification. C'est cohérent avec 1.2/1.5, où l'opération est
également rejetée. Si vous préférez conserver le repli, il se remet en place
facilement — dites-le.

Deux tests suivent :
logStatusNotification 1-6 request sends DiagnosticsStatusNotification by default
devient diagnosticsStatusNotification 1-6 request sends DiagnosticsStatusNotification, plus un test de rejet sans extensions, côté adapter
et côté toolkit.

Audit de couverture

Un passage systématique sur chaque modification a trouvé quatre comportements sans
test — dont deux qu'aucune revue n'aurait attrapés à la lecture :

• CSMSApi.send dispatche sur le type de requête avec un else fourre-tout :
  une branche oubliée ne casse pas la compilation, elle dégrade en
  IllegalStateException opaque à l'exécution. La branche ajoutée n'était couverte
  par rien, et generic-api n'avait aucun source set de test. C'est corrigé, et
  supprimer la branche fait bien échouer le nouveau test.
• DefaultCSMSApi n'avait aucun test. La délégation est couverte, ainsi que le
  caractère optionnel du callback.
• ApiFactory mappe la version de transport vers l'enum de sous-protocole WAMP
  à la main, et seules 1.6 et 2.0 se connectaient en WebSocket dans les tests : une
  entrée 1.5 inversée passait inaperçue. Le sous-protocole négocié est maintenant
  vérifié pour les trois versions — là aussi, l'inverser fait échouer le test.
• Les tests diagnostics 1.2/1.5 n'exerçaient que Uploaded et Uploading ; ils
  couvrent désormais les deux états terminaux et les deux transitoires, et
  vérifient le statut réellement transmis.

Une correction sur ma réponse au fil ApiFactory.kt:91

J'y ai écrit, et mis dans le code, « OCPP 1.2 has no WebSocket transport ».
C'est faux au sens de la spécification. La spec OCPP-J 1.6 définit le transport
JSON/WebSocket comme indépendant du contenu des messages et le déclare applicable
aux versions antérieures : sa table des sous-protocoles liste ocpp1.2 et
ocpp1.5, tous deux enregistrés à l'IANA, et son glossaire donne « OCPP1.5J » en
exemple. ocpp1.2 est donc légitime.

La vraie raison du rejet est une limite d'implémentation : il n'existe pas de
module ocpp-1-2-json, #84 demandant « SOAP only ». Le message sera reformulé en
ce sens. Un module ocpp-1-2-json fera l'objet d'une PR séparée : les 18 messages
1.2 sont un sous-ensemble strict des 24 messages 1.5, et les schémas sont
dérivables du WSDL 1.2 officiel — donc vérifiables, ce qui n'était pas le cas des
schémas 1.5 actuels.

Vérification

./gradlew build et la suite complète sont verts.

[juherr]

Mise à jour rapide : #97 étant fusionnée, la branche est rebasée sur dev et
l'empilement est terminé. La PR retombe à 22 commits et son diff ne contient plus
que le travail 1.2/1.5 — les réserves sur l'ordre de fusion et sur la base non
re-ciblable, dans mes deux commentaires précédents, sont caduques.

Le rebase a demandé une adaptation : dev a intégré
feat!: migrate public time API to kotlin.time, qui retire kotlinx-datetime de la
convention Gradle partagée. Les modules 1.2/1.5 ne compilaient plus ; ils utilisent
maintenant kotlin.time, et le décalage d'une milliseconde sur l'horodatage du
StatusNotification passe par une Duration, comme l'adapter 1.6 le fait déjà.
C'est isolé dans un commit dédié.

Un passage de nettoyage a suivi, sur le code ajouté depuis la revue :

• convertUnit clonait le tableau de l'enum UnitOfMeasure à chaque sampled
  value avant de sonder une seconde table d'alias ; une table unique construite
  une fois ramène ça à un seul accès de hash.
• Les deux catch de meterValues avaient des corps identiques, dans les deux
  adapters ; ils passent par un helper nommé.
• DiagnosticsStatusNotificationMapper.isSupported redisait à la main la liste de
  la branche throw, dans un setOf réalloué à chaque appel et tenu cohérent par
  un simple commentaire. C'est un when exhaustif désormais : le compilateur
  oblige à classer toute nouvelle valeur, au lieu qu'elle passe silencieusement
  pour supportée.
• getLocalIdByTransactionId copiait toute la map dans une liste de paires avant
  de la parcourir ; itérer la vue entries parcourt pareil sans allouer.
• Côté tests : littéral de requête factorisé, stubs morts retirés (leur propre
  verify(exactly = 0) prouvait qu'ils étaient inatteignables), et assertions qui
  ne s'écrasent plus en Boolean — ce qui masquait le message réel en cas d'échec.

Enfin, un !! résiduel a été retiré de GetConfigurationMapper : il suivait un
test de nullité, exactement le motif visé par la remarque SonarCloud. SonarCloud ne
l'avait pas signalé — techniquement le !! n'y était pas inutile, readonly
étant un val d'un autre module — mais l'intention de la remarque s'y appliquait.
Il ne reste aucun !! dans les modules de cette PR.

Point laissé en l'état volontairement : le message
"OCPP 1.2 has no WebSocket transport", que j'ai reconnu plus haut comme inexact au
regard de la spec OCPP-J. Il sera corrigé par la PR ocpp-1-2-json, qui supprimera
la garde elle-même — le reformuler ici serait du travail jeté.

./gradlew build et la suite complète sont verts.

[pbourseau]

Nouvelle passe de revue sur 0c5fe5ae.

Les points du tour précédent que j'avais relevés sont bien traités : convertReadingContext / convertLocation / convertMeasurand sont désormais des when exhaustifs avec rejet explicite (1.5), MeterValuesMapper.genToCoreReq (1.2) annonce le bon type d'exception et l'adaptateur l'attrape, getWampVersion remplace le valueOf opaque sur OCPP_1_2 + WEBSOCKET, et le +1ms ne mute plus la requête de l'appelant.

Il reste 4 remarques ci-dessous, dont une qui casse un cas nominal du protocole 1.2.

Vérifié par ailleurs et sain : les namespaces SOAP 1.2 (urn://Ocpp/{Cp,Cs}/2010/08/) et l'ordre des champs des mixins correspondent aux séquences du XSD 1.2 ; les tables de dispatch Actions / RealCSMSOperations / RealChargePointOperations sont complètes pour 1.2 ; les Enum.valueOf restants (Reset, CancelReservation, ReserveNow, AuthorizationStatus, SendLocalList) reposent sur des enums qui se correspondent effectivement.

[pbourseau]

NPE sur une BootNotificationResp 1.2 sans currentTime — cas nominal du protocole.

core12.BootNotificationResp déclare, à juste titre, currentTime: Instant? = null et heartbeatInterval: Int? = null : en OCPP 1.2 les deux ne sont obligatoires que quand le statut est Accepted, contrairement à 1.5/1.6. Mais la cible générique api.model.bootnotification.BootNotificationResp a currentTime: Instant et interval: Int, tous deux non-null.

Ce coreToGenResp étant abstract, MapStruct génère une assignation directe, et le contrôle de nullité Kotlin du constructeur lève.

Scénario : un central system 1.2 répond

<bootNotificationResponse><status>Rejected</status></bootNotificationResponse>

ce qui est légal et c'est la forme courante d'un refus. Ocpp12Adapter.bootNotification lève alors NullPointerException au lieu de retourner un Rejected exploitable. Vérifié : mapper.coreToGenResp(BootNotificationResp(status = RegistrationStatus.Rejected)) → NPE.

Suggestion : écrire un coreToGenResp explicite qui fournit un repli pour les deux champs (par ex. currentTime = maintenant, interval = 0 quand le statut n'est pas Accepted), ou rejeter avec un message clair. Aucun test ne couvre ce chemin aujourd'hui — les cas Rejected / NotSupported mériteraient d'être ajoutés.

[juherr]

Confirmé et corrigé (b5f00626), avec la spec à l'appui : le WSDL 1.2 déclare bien les deux champs optionnels, contrairement à 1.5.

1.2  <s:element name="currentTime"       type="s:dateTime" minOccurs="0" maxOccurs="1" />
     <s:element name="heartbeatInterval" type="s:int"      minOccurs="0" maxOccurs="1" />
1.5  <s:element name="currentTime"       type="s:dateTime" minOccurs="1" maxOccurs="1" />
     <s:element name="heartbeatInterval" type="s:int"      minOccurs="1" maxOccurs="1" />

coreToGenResp est désormais explicite. J'ai retenu le repli plutôt que le rejet : lever ferait perdre à l'appelant l'information la plus utile du message — le refus lui-même. currentTime retombe sur l'horloge locale, interval sur 0, ce qui se lit correctement comme « pas de heartbeat ». En revanche un Accepted sans ces champs est une faute du central system, donc il émet un warn.

1.5 est laissé tel quel, à dessein : son modèle core15 a les deux champs non-null, ce qui correspond à son propre schéma — l'abstraction MapStruct y est correcte.

Tests, écrits en rouge d'abord : un Rejected nu passe et reste exploitable (il levait une NPE avant le correctif), et un Accepted complet préserve les valeurs reçues, pour verrouiller le fait que le repli ne les écrase pas.

[pbourseau]

Des MeterValues 1.2 valides sont abandonnées à cause d'une résolution de transactionId dont 1.2 n'a pas besoin.

Ce bloc vient de 1.6, où MeterValuesReq porte un transactionId. En 1.2, MeterValuesMapper.genToCoreReq construit MeterValuesReq(connectorId = …, values = …) et ne lit jamais transactionId : le message OCPP 1.2 n'a pas ce champ.

Du coup le getTransactionIdsByLocalId ne fait que produire des modes de panne. Une MeterValues portant un transactionId absent du repository en mémoire — après un redémarrage du process, ou pour une transaction démarrée hors-bande — lève IllegalStateException("key … not found"), désormais attrapée, et la requête part en NOT_SEND : elle n'est jamais envoyée alors qu'elle est parfaitement transmissible. Une mesure de consommation est perdue.

Suggestion : supprimer la résolution en 1.2 et ne garder que mapper.genToCoreReq(request) dans le try (le mapping peut toujours échouer légitimement). Ne concerne pas 1.5, où MeterValuesReq a bien un transactionId.

[juherr]

Corrigé (b5f00626), c'est bien un héritage du 1.6 qui n'a aucun sens ici.

Vérifié : core12.MeterValuesReq n'a que connectorId et values, et MeterValuesMapper.genToCoreReq ne lit jamais transactionId. La résolution ne pouvait donc que produire des pannes, sans rien apporter. Le try ne couvre plus que le mapping.

Effet de bord agréable : IllegalStateException n'avait plus qu'une seule source dans ce bloc, le lookup ; son catch disparaît avec lui. Il ne reste qu'un catch (IllegalArgumentException), celui du mapping, qui reste légitime.

1.5 est inchangé : son MeterValuesReq porte bien un transactionId, donc la résolution y a un sens et son catch sur les deux types reste nécessaire.

Test : une MeterValues dont le transactionId est inconnu du repository part maintenant sur le fil (SUCCESS + verify(exactly = 1)), là où elle finissait en NOT_SEND.

[pbourseau]

EVConnected + Ended → Available annonce le connecteur libre alors que le véhicule est encore branché.

Le mapper 1.6 dont celui-ci dérive envoie cette combinaison sur Finishing, c'est-à-dire explicitement pas disponible : la transaction est terminée mais le câble est toujours en place.

En traduisant vers Available, le CSMS considère le connecteur libre et peut le proposer ou le réserver à un autre conducteur avant le débranchement.

Le downgrade fidèle en 1.2 est Occupied : ChargePointStatus n'a pas de Finishing, mais Occupied conserve la sémantique « pas encore réutilisable ». Le passage à Available viendra du ChargingStateEnumType.Idle qui suit le débranchement, déjà géré ligne 84.

[juherr]

Corrigé (b5f00626). Ton analyse est juste, y compris sur la conséquence : annoncer Available alors que le câble est branché invite le CSMS à proposer ou réserver le connecteur.

EVConnected mappe maintenant vers Occupied quel que soit le type d'événement, et le passage à Available vient bien du ChargingStateEnumType.Idle qui suit le débranchement.

Une conséquence à signaler : wrapper.type n'est du coup plus lu en 1.2 ni en 1.5. Je l'ai gardé volontairement, pour deux raisons. Le ChargingStateWrapper reste aligné sur celui de 1.6, où le type d'événement est réellement discriminant (Started -> Preparing, Ended -> Finishing). Et surtout, le supprimer rendrait le test de non-régression inexprimable : c'est précisément la combinaison EVConnected + Ended qu'il faut pouvoir épingler. Dis-moi si tu préfères quand même le retirer.

Test écrit en rouge d'abord, en 1.2 et en 1.5.

[pbourseau]

Même mapping EVConnected + Ended → Available qu'en 1.2.

Même analyse que sur adapter12/StatusNotificationMapper.kt : core15.ChargePointStatus dispose de Occupied, qui reflète correctement « transaction terminée, EV toujours branché ». Available sera émis au Idle suivant (ligne 83).

[juherr]

Corrigé (b5f00626) en même temps que le 1.2, avec le même raisonnement — core15.ChargePointStatus a bien Occupied.

Détail utile pour la revue : 1.5 a en plus Reserved, mais ce n'est pas la bonne cible ici (elle correspond à une réservation, pas à une transaction terminée câble branché). Occupied reste le downgrade fidèle de Finishing.

Test symétrique de celui du 1.2, rouge avant correctif.

[pbourseau]

Quatrième passe, sur b5f00626.

Les quatre points du tour précédent sont traités, et je les ai revérifiés dans le code plutôt que sur parole :

• BootNotificationMapper.coreToGenResp (1.2) est désormais explicite, avec le repli documenté et le warn sur un Accepted incomplet. Le choix du repli plutôt que du rejet est le bon : perdre le refus lui-même aurait été pire que la valeur manquante.
• Le lookup transactionId a disparu de Ocpp12Adapter.meterValues, avec le catch (IllegalStateException) devenu inutile qui part avec lui. 1.5 le garde à juste titre, son MeterValuesReq porte bien le champ.
• EVConnected -> Occupied en 1.2 comme en 1.5, quel que soit le type d'événement.

Rien de bloquant ce tour-ci. Six remarques, dont la première est la seule qui mérite vraiment une décision — les autres sont de la finition.

[pbourseau]

Changement de comportement runtime pour les stations 1.6 sans whitepaper, sans trace dans la PR.

Avant cette PR, logStatusNotification avec securityExtensions = false partait sur le flux core et fonctionnait :

} else {
    val mapper: DiagnosticsStatusNotificationMapper = ...
    val response = operations.diagnosticsStatusNotification(meta, mapper.genToCoreReq(request))
    ...
}

Maintenant l'appel passe inconditionnellement par securityOperations("LogStatusNotification"), dont le checkNotNull (l.105-108) lève IllegalStateException("LogStatusNotification requires the OCPP 1.6 security whitepaper, which is disabled on this charge point").

Le découpage est le bon — je suis d'accord avec le raisonnement du commentaire, et le test logStatusNotification is rejected without the security extensions montre que c'est assumé. Ce que je relève est l'absence de signal pour l'utilisateur : le changement est compatible à la compilation et casse à l'exécution. Un intégrateur qui appelait logStatusNotification sur une station 1.6 non-whitepaper compile sans un avertissement et découvre l'exception en production.

Aucun CHANGELOG, README ou fichier .md n'est touché par la PR, et son titre annonce « add OCPP 1.2 support » — personne ne va y chercher une rupture sur le chemin 1.6, qui est le plus utilisé.

Deux sorties possibles, au choix :

• une note de migration explicite (description de PR + notes de version) : « appelez désormais diagnosticsStatusNotification pour le flux core GetDiagnostics » ;
• ou garder le repli vers diagnosticsStatusNotification quand securityExtensions = false, avec un @Deprecated sur ce chemin, pour laisser un cycle de dépréciation.

La première me va très bien si tu préfères la coupe nette.

[juherr]

Tu as raison sur le fond : le changement était assumé dans le code et dans les tests, mais invisible pour qui lit la PR. J'ai pris la coupe nette avec la note de migration.

La description de la PR a désormais une section Breaking changes placée juste après le résumé, qui ouvre sur le fait que les deux ruptures compilent et cassent à l'exécution — c'est le point que tu soulèves, et c'est ce qui mérite l'avertissement. Elle donne le remplacement en deux lignes :

// avant — flux core GetDiagnostics, whitepaper désactivé
csmsApi.logStatusNotification(meta, LogStatusNotificationReq(status = Uploaded))

// après
csmsApi.diagnosticsStatusNotification(meta, DiagnosticsStatusNotificationReq(status = Uploaded))

J'y ai ajouté la seconde rupture, que je n'avais signalée que dans un message de commit : CSMSApi gagne une méthode abstraite, donc toute implémentation hors dépôt doit l'ajouter. CSMSApiCallbacks a un corps par défaut et n'est pas concerné.

Pas de CHANGELOG.md dans le dépôt, et le README ne documente pas les opérations une à une — la description de la PR est donc la seule surface de notes de version. Si vous en tenez une ailleurs pour la release, dis-moi où et je l'y reporte.

J'ai écarté la piste @Deprecated avec repli : elle demanderait de garder les deux sémantiques en parallèle sur la même opération, c'est-à-dire exactement l'ambiguïté que ce découpage supprime — et le repli resterait silencieux, donc personne ne migrerait avant sa suppression.

[pbourseau]

Le découpage diagnostics n'est fait que dans un sens, et le requestId = 1 en dur le rend visible.

La PR sépare bien les deux flux dans le sens CS -> CSMS : diagnosticsStatusNotification (core) d'un côté, logStatusNotification (whitepaper) de l'autre. Mais dans le sens CSMS -> CS, getDiagnostics reste projeté sur le générique getLog, ici comme en 1.5 et en 1.6.

Conséquence pour une implémentation générique de CSApi :

• à l'aller elle reçoit un GetLogReq et ne peut pas savoir si c'est un GetDiagnostics core ou un GetLog whitepaper — le seul indice est logType = DiagnosticsLog, qu'un vrai GetLog peut porter aussi ;
• au retour elle doit maintenant distinguer les deux, puisqu'ils arrivent sur deux opérations différentes ;
• et elle ne peut pas corréler les deux, le nouveau DiagnosticsStatusNotificationReq ne portant pas de requestId.

Le requestId = 1 en dur aggrave le tableau : toutes les requêtes GetDiagnostics de toutes les bornes partagent le même identifiant, donc un CSApi qui indexe ses demandes de log par requestId les écrase entre elles.

Le requestId = 1 est copié du mapper 1.6 pré-existant, donc la dette n'est pas de toi — mais la PR la duplique en 1.2 et en 1.5, et surtout c'est elle qui rend l'asymétrie structurante en créant l'opération générique manquante d'un seul côté.

Pas un bloquant pour cette PR. Mais est-ce que tu vois un GetDiagnostics générique arriver plus tard pour fermer la symétrie, ou est-ce que le choix est de laisser l'aller sur getLog définitivement ? La réponse change ce qu'il faut documenter.

[juherr]

Bonne question, et la réponse est oui : la symétrie devrait être fermée par un getDiagnostics générique, mais pas dans cette PR.

Le raisonnement est le même que pour diagnosticsStatusNotification — deux messages distincts ne doivent pas partager une opération générique. Mais fermer l'aller touche CSApi, donc une seconde addition abstraite à une interface publique, dans une PR qui en porte déjà une et qui a quatre tours de revue. Ça mérite sa propre PR, avec sa propre note de rupture. C'est cohérent avec le découpage de la PR ocpp-1-2-json déjà en attente.

Sur le requestId = 1, j'ai regardé de plus près et je ne pense pas qu'un identifiant unique soit la bonne correction. Ni GetDiagnostics ni la DiagnosticsStatusNotification qui lui répond ne portent d'identifiant sur le fil : générer une valeur unique suggérerait une corrélation que le protocole ne permet pas, ce qui est pire que la constante visible. La vraie conclusion est qu'un CSApi ne doit pas indexer ses demandes de log par requestId sur ce flux.

J'ai donc documenté la limite en KDoc sur les deux mappers 1.2 et 1.5 (f8030139) plutôt que de fabriquer un faux identifiant. Je n'ai pas touché au 1.6, pré-existant et hors périmètre — mais la même note s'y appliquerait.

[juherr]

Suivi tracé : #109.

Il reprend l'asymétrie et les trois conséquences que tu listes pour une implémentation générique de CSApi, avec les références précises des trois mappers, et acte la direction — un getDiagnostics générique sur CSApi, symétrique de ce que #98 a fait avec diagnosticsStatusNotification sur CSMSApi.

Il enregistre aussi le raisonnement sur le requestId, pour qu'il ne soit pas re-tranché à l'envers plus tard : la constante n'est pas un bug à corriger par un identifiant unique, faute de corrélation possible sur le fil. Et il note un point que ta remarque m'a fait voir — le mapper 1.6 porte la même limite sans la documenter, contrairement aux 1.2/1.5.

Une question ouverte y est laissée : si getDiagnostics devient générique, faut-il que getLog rejette en 1.2/1.5, comme logStatusNotification le fait désormais ?

[pbourseau]

Dernier else -> valueOf parmi les convertisseurs de downgrade.

Toute la PR a convergé vers des when exhaustifs sur les conversions générique -> cible, précisément pour que l'ajout d'une valeur au modèle générique casse la compilation plutôt que la production. Ce convertisseur est le seul qui garde le repli, et le convertResetType juste en dessous est exhaustif — le contraste est dans le même fichier.

Ça marche aujourd'hui parce que ResetStatusEnumType vaut exactement {Accepted, Rejected, Scheduled} et que Scheduled est déjà traité au-dessus. Mais c'est un équilibre que rien ne protège : une quatrième valeur ajoutée au générique compile sans bruit et lève IllegalArgumentException: No enum constant à la première réponse de reset qui la porte.

Vu que Scheduled est déjà sorti, c'est deux lignes :

ResetStatusEnumType.Accepted -> ResetStatus.Accepted
ResetStatusEnumType.Rejected -> ResetStatus.Rejected
ResetStatusEnumType.Scheduled -> ResetStatus.Accepted

Je distingue bien ce cas des valueOf restants sur les conversions montantes (AuthorizationStatusEnumType, RegistrationStatusEnumType, CancelReservationStatus...) : là c'est la cible qui est le sur-ensemble, le risque n'est pas le même et je ne demande rien dessus.

[juherr]

Corrigé (f8030139), en 1.2 et en 1.5 d'un coup. Tu as raison sur l'analyse : ça ne marchait que par coïncidence, Scheduled étant déjà sorti au-dessus.

ResetStatusEnumType.Accepted -> ResetStatus.Accepted
ResetStatusEnumType.Rejected -> ResetStatus.Rejected

// OCPP 1.2 has no Scheduled: the reset is acknowledged, so Accepted is the honest downgrade.
ResetStatusEnumType.Scheduled -> ResetStatus.Accepted

Vérifié que la garantie est réelle et pas seulement déclarée : en retirant la branche Rejected, la compilation échoue avec 'when' expression must be exhaustive. Add the 'Rejected' branch or an 'else' branch.

Sur les tests, une précision d'honnêteté : ce correctif ne change aucun comportement observable, donc il n'y avait pas de test rouge à écrire — la garantie est portée par le compilateur, pas par un test. En revanche l'audit a montré un vrai trou de couverture : seul Accepted était exercé, via CSApiAdapterTest. Rejected et surtout la dégradation Scheduled -> Accepted ne l'étaient pas. Les trois sont épinglés maintenant, en 1.2 et en 1.5.

Et merci d'avoir distingué explicitement les valueOf montants : c'est la bonne ligne de partage, et c'est celle que j'avais retenue.

[pbourseau]

Même remarque qu'en 1.2, le fichier est identique — core15.ResetStatus a lui aussi exactement {Accepted, Rejected}. À traiter d'un coup avec l'autre.

[juherr]

Traité avec le 1.2 dans f8030139 — même correction, même test épinglant les trois statuts. Détail : core15.ResetStatus vaut bien {Accepted, Rejected} lui aussi, donc Scheduled -> Accepted y est la même dégradation.

[pbourseau]

Tu avais signalé toi-même que type n'est plus lu depuis le correctif EVConnected -> Occupied, et tes deux raisons de le garder me vont : l'alignement sur le wrapper 1.6, où le type est réellement discriminant, et le test qui perdrait son sens.

Je ne demande donc pas de le retirer — juste une ligne de KDoc sur le champ pour dire qu'il n'est pas consommé en 1.2/1.5 et pourquoi. Sans ça, le prochain lecteur qui voit createChargingStateWrapper(..., statusReq.getEventType()) passer un argument que rien ne lit va le traiter comme un bug et « corriger » l'expression MapStruct. Idem côté 1.5.

[juherr]

Bien vu, c'est exactement le risque. KDoc ajouté sur le champ en 1.2 et en 1.5 (f8030139) :

/**
 * Not read by [convertChargingState] in OCPP 1.2: unlike 1.6, which splits EVConnected into
 * Preparing and Finishing on the event type, 1.x maps it to Occupied whatever the event. The
 * field is kept so this wrapper stays shaped like the 1.6 one, and so tests can pin the
 * combination that used to yield Available on Ended.
 */

Je l'ai mis sur le champ plutôt que sur la fonction, pour qu'il soit sous les yeux de quiconque suit l'argument depuis l'expression MapStruct.

[pbourseau]

Ce test et logStatusNotification request sends LogStatusNotification when the security extensions are on (l.967) sont le même test.

Même adaptateur securityExtensions = true, même LogStatusNotificationReq(Uploaded, requestId = 1), même stub sendMessageClass(any(), "LogStatusNotification", any()), mêmes assertions. Le diff entre les deux blocs ne rend que le nom et le passage à la ligne des arguments du every.

C'est un artefact du découpage : l'ancien logStatusNotification request couvrait le chemin core (securityExtensions = false), il a été basculé sur true pendant la refonte, et le nouveau test nommé explicitement fait maintenant doublon avec lui.

Supprimer l.932-951 et garder le nom explicite. Les trois tests qui restent couvrent alors exactement les trois cas : whitepaper actif, whitepaper absent, flux core via diagnosticsStatusNotification.

[juherr]

Confirmé et supprimé (f8030139). C'est bien un artefact du rebase : logStatusNotification request était l'ancien test du chemin core, que j'ai basculé sur securityExtensions = true pendant la refonte — sans voir qu'il devenait le doublon du test nommé explicitement, arrivé par la branche whitepaper.

J'ai gardé le nom explicite, comme tu le proposes. Les trois cas restants couvrent bien les trois chemins : whitepaper actif, whitepaper absent (rejet), et flux core via diagnosticsStatusNotification — ce dernier balayant les quatre statuts, puisque 1.6 les modélise tous.

[sonarqubecloud]

Quality Gate passed

Issues
10 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
20.8% Duplication on New Code

See analysis details on SonarQube Cloud