izivia / ocpi-toolkit Goto Github PK

Hi all.

I'm wondering why all the endpoints of CredentialsServer are registered with the secured flag to false.

Originally posted by @xhanin in #14 (comment)

I realize that the use of !! will make error reporting very poor if those headers are missing. we should rather raise a custom exception and return the appropriate error response. This is not part of this PR scope, but we should create an issue for that

The OCPI documentation states that:

When the server receives a valid OCPI object it SHOULD respond with:
• HTTP 200 - Ok when the object already existed and has successfully been updated.
• HTTP 201 - Created when the object has been newly created in the server system.

But the HTTP code is generated based on the OCPI status, so it's difficult to know which HTTP status must be returned.

Currently the library only stores a platforms tokens, combined with the versions URL as identifier. While this is sufficient for authentication of a remote party, it isn't enough to identify them. We currently workaround this issue by storing the platforms country_code and party_id when storing TOKEN_A, though this could be handled by the library as part of the registration process, by downloading the other platforms Credentials once tokens have been exchanged.

The reason we need this information, is to identify calls made to Session endpoints, so we can filter return values based on the caller (eg: only return Sessions, that were initiated by Tokens owned by the calling eMSP). Similarly, when pushing Session objects to an eMSP, at the time of creating the Session object, all we have are the identifiers of that platform, and not its base URL.

The simplest solution would be, to keep the platformURL as identifier and add methods to the PlatformRepository interface for associating the identifiers:

    suspend fun getPlatformUrlByIdentifiers(country_code: String, party_id: String): String?
    suspend fun savePlatformIdentifiers(platformUrl: String, List<IdentifierPair>: String)

The documentation states that (8.2.2.2. PUT Method):

When the PUT only contains a Connector Object, the Receiver SHALL also set the new last_updated value on the parent EVSE and Location Objects.

When the PUT only contains a EVSE Object, the Receiver SHALL also set the new last_updated value on the parent Location Object.

But the toolkit is not doing that. This can be done in the repository implementation, letting the users of the library to implement it, but I think it's not the best approach.

What do you think?

CdrLocation uses String instead of GeoLocation for coordinates

https://github.com/atschabu/ocpi-toolkit/blob/28624249bac38ef148f9c7269d0b9818e7de8112/ocpi-toolkit-2.2.1/src/main/kotlin/com/izivia/ocpi/toolkit/modules/cdr/domain/CdrLocation.kt#L38

data class CdrLocation(
    val id: CiString,
    ...
    val coordinates: String,
    ...
)

Hello.

I'm trying to implement some custom logic in the Locations service, so my first idea was to inherit from LocationsEmspService, call the super and implement my own logic. But the Service classes are closed, so it's impossible to inherit from them.

Should we open these classes to allow the inheritance or force users to implement a new class?

Maybe there is another option that I don't see. Please, feel free to comment.

Thanks.

Current service interfaces are missing context information like

• partnerUrl
• routing headers (indicating which country_code, party_id it is intended for in a multi party implementation)
• ... might be missing something else

In order to get to this information, it would be required to reimplement the server itself, which would somewhat defeat the purpose of the lib. To make the lib slightly more extensible, I can think of two ways of solving this

a) one extra layer of abstraction between server and service that has full access to the request, but most of the tedious work of deserializing body, defining paths, and variables is done. A default implementation could do validation, before handing off to service/repo interface.

b) Include a "context" object in all service interface methods (this is a very golang thing to do ... there might be a more elegant solution for kotlin) that includes the headers map, partnerUrl, and possibly more fields in the future.

After unregistration, CredentialsServer return null. Then, in OcpiResponseBody l.124, when the OcpiStatus is SUCCESS, and the returned data is null, we set the HTTP status to 404.

I think it should rather be 200

I was thinking about these 2 fields in OcpiResponseBody that are in snake_case and I was wondering why they are not in camelCase like the rest of the variables.

Hello,

I want to start a discussion (again 😄 ) about the use of CPO & eMSP vs. Sender & Receiver.

As an example, I'm currently implementing the ChargingProfiles module which allows different topologies:

• eMSP <-> CPO
• SCSP <-> eMSP
• SCSP <-> CPO

As you can see, the eMSP can act as a Sender or as a Receiver, so naming ChargingProfilesEmspServer will be very confusing.

headers = (ocpiResponseBody as OcpiResponseBody<SearchResult<*>>)
        .getPaginatedHeaders(request = this)
)

has to be replaced by:

headers = headers + (ocpiResponseBody as OcpiResponseBody<SearchResult<*>>)
        .getPaginatedHeaders(request = this)
)

will be fixed in the same time as fix of #62

The definition for CredentialsRole. business_details refers to definition in Location module. The following class and its dependencies can be removed from credentials module

As far as I can see, all generated Partials, use Partials as field values. For example SessionPartial.charging_periods is of type List<ChargingPeriodPartial>?, but the expectation is that we provide a valid ChargingPeriod, not partially updating existing ChargingPeriods (as that would require a PUT Session call)

When a PATCH request contains the charging_periods field (inside a Session object), this SHALL be processed as a request to add all the ChargingPeriod objects to the existing Session object.

The issue is worse with the Location object, where we have about 10+ fields that are complex objects. Unless I interpret the protocol incorrectly, only the provided root object can be Partial, while all field values should be "regular" fully valid objects. Therefore types like GeoLocation and BusinessDetail don't even need to be annotated as @Partial, and generated Partials should not use Partial fields as field types. Which means a LocationPartial should contain a list of Evse, and only when calling PATCH Evse endpoint should we using EvsePartial, etc.

When error are thrown, it's hard to debug because we do not know which property check failed, and what value it had

Will be fixed in #72

• When a request contains payload, the "Content-type: application/json" header has to be present
• All responses should have that header

Somewhat annoyed at myself for not noticing this while having worked on the validation issue ... but here we go .. one more single line issue + PR 😆

Session.kwh is a number field (with decimals) but is currently using Int ... will be switching it to BigDecimal, which also brings it in sync with CDR.totalEnergy field.

When 2 parties don't find any matching mutual version, a 3002 OCPI error must be generated.

In CredentialsServerService.findLatestMutualVersionAndStoreInformation an OcpiServerNoMatchingEndpointsException (OCPI 3003) is thrown.

An OcpiServerUnsupportedVersionException (OCPI 3002) must be thrown.

I've been evaluating this library and found an inconsistency with the credentials module. According to the documentation

The Receiver stores CREDENTIALS_TOKEN_B and uses it for any requests to the Sender Platform, including the version information and details

yet it looks like this library is discarding CREDENTIALS_TOKEN_B together with TOKEN_A on successful registration, and is only using CREDENTIALS_TOKEN_C for receiving and sending requests:

Shouldn't the B token be kept, and the credentials include some kind of hint if the platform is either the receiver or sender?

some calls incorrectly use query parameters instead of path segments

Looking at location and session service implementations, we currently only validate the from/to dates if both are set. Specification allows for none, either or both to be defined. Instead of checking only if both are defined, we should validate them individually

When requesting a paginated endpoint, if the response includes the header Link for the next page, but the value is left empty (or includes invalid content), an uncontrolled IndexOutOfBoundsException is thrown.

We could enhance OcpiModuleServer::registerOn(transportServer) to automatically include the newly registered module with its base path to the endpoints list.

Currently we need to manually curate list of Endoints

                Endpoint(
                    identifier = ModuleID.credentials,
                    role = InterfaceRole.RECEIVER,
                    url = "${config.getBaseUrl()}/${VersionNumber.V2_2_1.value}/credentials",
                ), Endpoint(
                    identifier = ModuleID.locations,
                    role = InterfaceRole.SENDER,
                    url = "${config.getBaseUrl()}/${VersionNumber.V2_2_1.value}/locations",
...

and provide this to a custom implementation of the VersionDetailsRepository

Instead we could provide ModuleId and role to OcpiModuleServer

abstract class OcpiModuleServer(
  val basePath: String,
  val module: ModuleID,
  val role: InterfaceRole,
  val versionDetailsRepo: MutableVersionDetailRepo,
) {
    protected val basePathSegments: List<PathSegment> get() = listOf(FixedPathSegment(basePath))

    protected abstract suspend fun doRegisterOn(transportServer: TransportServer)

    suspend fun registerOn(transportServer) {
      versionDetailsRepo.addEndpoint(...)
      doRegisterOn()
  }
}

Where MutableVersionDetailRepo holds the baseUrl already and implements VersionDetailsInterface

In a PATCH there may be only one field, so the SessionPartial class should be used.

startDate can never be greater than current time, as we would always have started in the past. Similar issue with endDate

And below it should be unique ID check

    validate(SessionPartial::evseId).isPrintableAscii().hasMaxLengthOf(36)

not

    validate(SessionPartial::evseUid).isEvseId()

See https://github.com/ocpi/ocpi/blob/release-2.2.1-bugfixes/mod_locations.asciidoc#143-capability-enum for reference

I wonder if we want to "allow" the use of tokenA, or actually force to use it? The only case of using tokenA is when we initialize the credentials handshake, where we call 3 endpoints with tokenA (and in that case we are sure we want to use tokenA). Then we will always use the client token. I will go on with the review to see if what I say makes sense or not.

Originally posted by @xhanin in #16 (comment)

Hi all.

I'm trying to understand how I can use the pagination when requesting info to the other part.

When I make the call, the response contains the nextPageUrl but I don't know how to use it in the next request.

Thanks a lot!!!

Use suspend functions the same way it's done in ocpi 2.2.1

cf: https://github.com/Kotlin/kotlinx-datetime

Both patchEvse and patchConnector are missing the ID for evse and connector in the path

https://github.com/IZIVIA/ocpi-toolkit/blob/486a2f3454ab6c185a10de8cfe221f29dbf9cc23/ocpi-toolkit-2.2.1/src/main/kotlin/com/izivia/ocpi/toolkit/modules/locations/LocationsCpoClient.kt#L188C1-L188C1

should be

      path = "/$countryCode/$partyId/$locationId/$evseUid",

should be

        path = "/$countryCode/$partyId/$locationId/$evseUid/$connectorId",

I could create PR for it, not sure what your contribution guidelines are

When the required headers X-Total-Count or X-Limit are not returned from the other partner, an uncontrolled NullPointerExeption is thrown.

Have you taken into account the connection to a multi-CPO or multi-eMSP platforms?

As I can see, the library is using the platformUrl as an identifier for finding the platform in the repo.

If a part wants to connect to another part that is in a platform that provides a service to many CPOs or eMSPs, the platformUrl may be the same, but the identifier will be the Authorization token and the country_code and party_id fields.

While testing this library, we also implemented the command module, following the existing structure as far as possible.

We would be happy to contribute it back. I understand that this might increase your maintenance burden, so I leave it to you to decide if or when you want to receive it.

For new users of the lib, it is not obvious how to filter result by partyId/countryCode. Currently, it is required to use:

currentRequestMessageRoutingHeaders()

To retrieve:

data class RequestMessageRoutingHeaders(
    val toPartyId: String? = null,
    val toCountryCode: String? = null,
    val fromPartyId: String? = null,
    val fromCountryCode: String? = null
)

The idea is to include countryCode / partyId in each method. Example:

// LocationsCpoInterface.kt

suspend fun getLocations(
    countryCode: String?,
    partyId: String?,
    dateFrom: Instant?,
    dateTo: Instant?,
    offset: Int = 0,
    limit: Int?
): OcpiResponseBody<SearchResult<Location>>

Or something like:

// ??.kt

data class PartnerIdentifier(
    val countryCode: String,
    val partyId: String
)

// LocationsCpoInterface.kt

suspend fun getLocations(
    partnerIdentifier: PartnerIdentifier?,
    dateFrom: Instant?,
    dateTo: Instant?,
    offset: Int = 0,
    limit: Int?
): OcpiResponseBody<SearchResult<Location>>

Created following #78

The platforms may require some endpoints to be available on the other platform.

I'm requesting proposals for approaches to implement it.

this is not really part of the scope of this PR, but adding those parameters to the withRequiredHeaders makes it more obvious: it's tedious and error prone to have all the clients call this method to add the headers. I think we should move the responsibility to the transportClient to add the required headers in the send method. But I tihnk we can move forward with this PR and address this point in another PR, as well as the review of how authentication and tokens are used / named.

Originally posted by @xhanin in #14 (comment)

It concerns

• .withRequiredHeader()
• .authenticate()

In CredentialsServerService, when trying to access to an unavailable (or returning errors) sender's server, an OcpiServerGenericException is thrown.

Specification:

When the server connects back to the client during the credentials registration, it might encounter problems. When this happens, the server should add the status code 3001 in the response to the POST from the client.

So an OcpiServerUnusableApiException must be thrown.

Hi there,

The lib got a lot of attention lately, so I would like to share some information with you :)

Context:

This lib was initiated in 2022 by the company Izivia which is a French eMobility company. The project was then deprioritized quite quickly, and there was a long period of inactivity. Recently, Izivia reprioritized the lib, and the goal is to have a "stable" version in mid 2024.

It is getting more and more attention, and there is no contribution guidelines yet (it will come soon), but we will review your PRs with pleasure.

Current state

As of today (2023, 1st of December), only the versions and credentials modules were tested with real partners. The next module will be location. The rest of the module were implemented following the OCPI protocol, but they are not properly tested yet. So you should expect to find a lot of bugs on these modules.

If you use the lib today, you have to expect to get breaking changes in the API in the following months. The overall approach will not change though, so breaking changes should be limited. Version 0.0.15 will bring a lot of changes, and its goal is to include most of the planned breaking changes (using camelCase kotlin code instead of snake_case, rename Platform to Partner and so on). In addition to that, I will update readme / set up a contribution guidelines for the repo to be more welcoming for future contributors. So once 0.0.15 will be released, there should not be any more big breaking changes. I plan on releasing it in 2-3 weeks.

Why should you consider using it / contribute ?

We are in communication with real OCPI partners, which allows us to receive real world feedback on the our lib. Currently, we are only testing the credentials and versions modules with real partners, and we are in the process of developing the location module, which we will soon test with a real partner too. The rest will come in the following months.

You can say anything below, this issue has been created to discuss about this library

Hey,

I have a java project that uses ocpi-toolkit and so far it is working great.

My question is, how to use ocpi-toolkit in java now that suspend functions are used?

I've read about java interoperability with suspend functions and it seems that the interoperability with suspend functions is not really great, or at least not great if ocpi-toolkit doesn't adapt suspend functions to be used in java(example making use of CompletableFuture or something.

Sources:

https://youtu.be/_hfBv0a09Jc?t=2159 - timestamped video talking little bit about Java interoperability KotlinConf 2017 - Introduction to Coroutines by Roman Elizarov

https://stackoverflow.com/questions/52869672/call-kotlin-suspend-function-in-java-class

I would rather use a dedicated parameter to tell if we are in the list of endpoints allowed in the handshake or not, to allow tokenA only there. I find this detection mechanism error prone, and as this is a security feature I think it's worth to make it more strict

Originally posted by @xhanin in #16 (comment)

I marked the issue with ocpi 2.1.1 and 2.1.1 gireve, but before applying this issue, they have to include #15

Currently, I have to create GH release by hand. The CD should do that on its own.

• Title: Version X.Y.Z
• Content: Automatically generated changelog
• Attached files: Produced .jar files (including sources)

If more information has to be given in the changelog, it will be done manually

Hi team,
Congratulations for the good job you're doing.

As I'm testing the library, I found some issues.

The return type should be nullable, because the session may not be found.

We currently disallow trailing commas, even though this might be one of the more useful Kotlin features, and is in line with trends in other modern languages like golang. Having trailing commas helps diffs staying smaller, and easier copy'n pasting of code and moving code lines up and down.

Unnecessary trailing comma before ")" (standard:trailing-comma-on-call-site)

I'd suggest we switch from disallowing trailing commas to requiring them.

As the documentation states:

Optional because: if an evse_id is to be re-used in the real world, the evse_id can be removed from an EVSE object if the status is set to REMOVED.

the field Evse.evseId should be nullable.

Trying to make a PATCH request to update a Location results in the following error:

ERROR OcpiResponseBody com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.izivia.ocpi.toolkit.modules.locations.domain.LocationPartial` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('{
"name": "Interparking Gent Zuid",
"last_updated": "2019-06-24T12:39:09Z"
}')
 at [Source: (String)""{\n\"name\": \"Interparking Gent Zuid\",\n\"last_updated\": \"2019-06-24T12:39:09Z\"\n}""; line: 1, column: 1]

I think that it's because the toolkit is trying to create a LocationPartial that have all the fields as optional, but without a default value, so is requiring all the fields to be present in the JSON input.

The HTTP response code is automated based on the OCPI response code:

Currently, the code returned is HTTP 500 on any response that is not SUCCESS or CLIENT_INVALID_PARAMETERS. So returning any of the other OCPI errors will result in an HTTP 500 error.

The documentation states that

If a request is syntactically valid JSON and addresses an existing resource, a HTTP error MUST NOT be returned. Those requests are supposed to have reached the OCPI layer.

so all these responses should be and HTTP 200.

I've asked for my thoughts on this in the OCPI Slack: https://ocpi.slack.com/archives/C0S64NZJQ/p1707299786355599
and confirmed by one of the authors of the OCPI documentation 😅

Maybe related with #65

As the documentation states:

If the header is missing or the credentials token doesn’t match any known party then the server SHALL respond with an HTTP 401 - Unauthorized status code.

But in the current implementation, the GET /versions with an incorrect CREDENTIALS_TOKEN_A is returning an HTTP 400.