feat(versions) #61: VersionDetails based on registered OcpiModuleServers #64

atschabu · 2024-01-12T06:01:24Z

Merges Versions and VersionDetails server together and ensures consistency between registered handlers and returned version information
OCPI servers register themselves with the VersionDetailsRepository, ensuring handlers and endpoint description has consistent path information

…leServers

atschabu · 2024-01-12T07:22:45Z

I can't see what SonarCloud is complaining about (no access)

lilgallon

Great job, I will test this new feature in our implementation to make sure I did not miss anything, but it looks good to me

lilgallon · 2024-01-12T08:41:03Z

ocpi-toolkit-2.2.1/src/main/kotlin/com/izivia/ocpi/toolkit/modules/versions/VersionsServer.kt

+        ) { req ->
+            req.httpResponse {
+                service.getVersionDetails(
+                    versionNumber = req.pathParams["versionNumber"]!!


Sonarcloud was complaining about that. I marked this issue as won't fix because handle implementation has the responsability to make sure that specified pathParams exist

lilgallon

I tested it against our multi tenant platform, and I found some issues

lilgallon · 2024-01-12T09:42:48Z

transport/src/main/kotlin/com/izivia/ocpi/toolkit/transport/TransportServer.kt

+    /**
+     * The configured baseUrl this server listens to, used to generate NextPageUrl and Endpoint Url
+     */
+    fun baseUrl(): String


In the lib, this field is only used by registerOn of OcpiSelfRegisteringModuleServer to specify the path of the endpoint of the module (Endpoint Url).

According to your TransportServer implementation, it can, indeed be the baseUrl used to generate NextPageUrl (as long as you pass it in the baseUrl field of the HttpRequest).

In our case, we have a multi tenant platform. So each platform can have its own baseUrl to contact us. So we dynamically compute the baseUrl every time we receive a request. In our case, we will not use baseUrl() and we will not use automatic endpoint registration because each tenant has its set of endpoints it can access.

Knowing that, I would like to change the documentation to say something like:

The configured baseUrl this server listens to. ONLY used by the lib to generate Endpoint Url. You can use it in handle to specify the baseUrl of HttpRequest. In that case, it will be used to generate NextPageUrl

Then, in the lib, we have to make sure that this baseUrl is not used anywhere else (the comment implies that it is not). If a baseUrl has to be used, it will be the one in HttpRequest

We can even rename it to staticBaseUrl(), but I am not sure about that

lilgallon · 2024-01-12T09:56:00Z

...olkit-2.2.1/src/main/kotlin/com/izivia/ocpi/toolkit/modules/credentials/CredentialsServer.kt

-    basePath: String = "/2.2.1/credentials"
-) : OcpiModuleServer(basePath) {
-    override suspend fun registerOn(transportServer: TransportServer) {
+    versionsRepository: MutableVersionsRepository,


versionsRepository: MutableVersionsRepository should be nullable and be null by default everywhere. So if one wants to opt in automatic endpoint registration they can, but it is not a must have.

In our case we will not use it because we have a list of available endpoints for each tenant

just to clarify, and mostly out of curiosity, why do you need various endpoints? Is it just so that it is not obvious to connecting parties that is a multi tenant system?

You could still benefit from this approach, by moving responsibility of adjusting endpoint URL's to the VersionServer. That's partly what I tried to achieve here too, move all the HTTP specific stuff as close as possible to the HTTP components, as a repository shouldn't need to know such specifics as our external facing URL (ideally that's just some config you pass in as an env var). In your case, where you pretend to be at a certain URL for certain incoming connections, you could store the external baseUrl within your partner repo, provide a placeholder value in the baseUrl, and then have the VersionsServer replace(placeholder, actualValue).

Once #62 is implemented, you could do it in the VersionsService

Each tenant has its sets of visible endpoints. We hold the tenant information in coroutines context. Moreover, we have separate server for registration and the rest. So the baseUrl of version / credentials is different from locations for instance.

Yes we could benefit from that, but we liked the fact that the way we implemented things was straightforward. We have a configuration that specifies with tenant can see what, and this config is only used at one place, in the VersionsRepository (all this logic is centralized in one place). We may change our minds later on, but as of now it works well that way

In addition to that, we do not have access of versions repository from our other server, so we can not use automatic endpoint registration. Yes we could call endpoints from our other server to our credentials server, but if it's only to perform automatic endpoint registration, it is not worth the cost of development.

But we understand that this feature is really relevant for a lot of other cases ;)

lilgallon · 2024-01-16T08:37:49Z

I will review your work asap today (I've been very busy yesterday)

lilgallon

Thank you for your improvements :)

I have a last request, and we will be good (I tested the lib with this change and it's working properly)

lilgallon · 2024-01-16T10:13:11Z

...t-2.2.1/src/main/kotlin/com/izivia/ocpi/toolkit/modules/versions/services/VersionsService.kt

 import com.izivia.ocpi.toolkit.modules.versions.repositories.VersionsRepository

 class VersionsService(
-    private val repository: VersionsRepository
+    private val repository: VersionsRepository,
+    private val baseUrl: String,


We should take advantage of coroutines here and use a baseUrlProvider (that is what is done in CredentialsServerService)

Suggested change

private val baseUrl: String,

private val baseUrlProvider: suspend () -> String,

It allows anyone to use coroutine context to hold information about baseUrl. That is what we do in our multi tenant platform

I have a feeling I'm missing an exciting feature of kotlin ... do you have any example of how to make use of coroutine context for this particular use case? Or will PR for #62 contain an example?

PR for #62 will contain examples, but we have something that looks like this:

VersionsService( repository = MyVersionsRepository(), baseUrlProvider = { config.ofCurrentTenant().baseUrl }, // ... )

lilgallon · 2024-01-16T10:13:26Z

...t-2.2.1/src/main/kotlin/com/izivia/ocpi/toolkit/modules/versions/services/VersionsService.kt

+        repository.getVersions().map {
+            Version(
+                version = it.value,
+                url = "$baseUrl$versionDetailsBasePath/${it.value}"


Suggested change

url = "$baseUrl$versionDetailsBasePath/${it.value}"

url = "${baseUrlProvider()}$versionDetailsBasePath/${it.value}"

sonarqubecloud · 2024-01-17T01:45:37Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

lilgallon

Thank you for your work !

atschabu · 2024-01-17T10:50:11Z

glad to improve this great library ... and thanks for considering it, even though you don't benefit from it (yet)

lilgallon · 2024-01-17T10:56:08Z

We are currently developing the Session module in our platform, so we already benefit from your contributions ;)

feat(versions) IZIVIA#61: VersionDetails based on registered OcpiModu…

dca9ee5

…leServers

lilgallon reviewed Jan 12, 2024

View reviewed changes

lilgallon requested changes Jan 12, 2024

View reviewed changes

atschabu added 2 commits January 15, 2024 09:57

update transportserver doc

a8f15e9

make mutableversionsrepo optional

c6b1d62

atschabu requested a review from lilgallon January 16, 2024 02:55

lilgallon requested changes Jan 16, 2024

View reviewed changes

provide baseUrl to VersionServer via function

a7d7ad2

atschabu requested a review from lilgallon January 17, 2024 01:49

lilgallon approved these changes Jan 17, 2024

View reviewed changes

lilgallon merged commit 34b9f03 into IZIVIA:main Jan 17, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(versions) #61: VersionDetails based on registered OcpiModuleServers #64

feat(versions) #61: VersionDetails based on registered OcpiModuleServers #64

atschabu commented Jan 12, 2024

atschabu commented Jan 12, 2024

lilgallon left a comment

lilgallon Jan 12, 2024

lilgallon left a comment

lilgallon Jan 12, 2024

lilgallon Jan 12, 2024

atschabu Jan 15, 2024

lilgallon Jan 16, 2024

lilgallon Jan 16, 2024

lilgallon commented Jan 16, 2024

lilgallon left a comment

lilgallon Jan 16, 2024

atschabu Jan 17, 2024

lilgallon Jan 17, 2024

lilgallon Jan 16, 2024

sonarqubecloud bot commented Jan 17, 2024

lilgallon left a comment

atschabu commented Jan 17, 2024

lilgallon commented Jan 17, 2024

	private val baseUrl: String,
	private val baseUrlProvider: suspend () -> String,

	url = "$baseUrl$versionDetailsBasePath/${it.value}"
	url = "${baseUrlProvider()}$versionDetailsBasePath/${it.value}"

feat(versions) #61: VersionDetails based on registered OcpiModuleServers #64

feat(versions) #61: VersionDetails based on registered OcpiModuleServers #64

Conversation

atschabu commented Jan 12, 2024

atschabu commented Jan 12, 2024

lilgallon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilgallon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilgallon commented Jan 16, 2024

lilgallon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 17, 2024

Quality Gate passed

lilgallon left a comment

Choose a reason for hiding this comment

atschabu commented Jan 17, 2024

lilgallon commented Jan 17, 2024