Object creation returns HTPP 200 instead of an HTTP 201

[andacata]

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.

ocpi-toolkit/ocpi-toolkit-2.2.1/src/main/kotlin/com/izivia/ocpi/toolkit/common/OcpiResponseBody.kt

Line 124 in ed450b6

OcpiStatus.SUCCESS.code -> if (ocpiResponseBody.data != null) HttpStatus.OK else HttpStatus.NOT_FOUND

[lilgallon]

I agress with you, there is a design issue here (#11 is also caused by this design issue). The real issue is that we do not have any context here.

In #11, we need something to say that even if we do not have any data, it should be HttpStatus.OK, and in your case, even if it's SUCCESS and there is some data, it has to be 201.

We need a way to pass the HttpStatus if we need something specific. And otherwise, we should use the existing logic. I am unsure what approach to use

Maybe

suspend fun <T> HttpRequest.httpResponse(fn: suspend () -> OcpiResponseBody<T>): HttpResponse =

should be changed to

suspend fun <T> HttpRequest.httpResponse(fn: suspend () -> Pair<OcpiResponseBody<T>, HttpStatus?>): HttpResponse =

In that case, we will need to update a lot of code.

Otherwise, one way to do that would be to use coroutine context, and pass HttpStatus if we want to override it. We will add support for adding stuff in a coroutine context soon

[andacata]

👍

I'll try to think of other ways to avoid having to change too much code.

[atschabu]

the easiest way I can see this working, is by adding another non-serialized field to the top level objects (location, evse, connector, session, ...). This will allow repository to set this field on creation, and then have httpserver respond with correct httpstatus. This would be perfectly backwards compatible, without a single interface having to change.

interface Createable { val isCreated: Boolean }
data class Location: Createable (
   ...
  override val isCreated: Boolean = false,
)

Though at this point I start having a slight issue with the lack of separation of domain model and wireformat. Up to this point our domain model, and therefore what is passed around through the entire system, was equivalent to the wireformat (what we serialize). The xxxPartial would be a possible candidate to be selected as wireformat (and in some instance already is), as it is primarily used at the edge for validation and de/serialization. Annotation processor can ignores that field by name.

That said, this feels like one of those parts of the spec that were added "for completeness sake". I can't really see a valid use case where a 200 instead of a 201 is invalid. Receiver might create objects on receiving a PUT or after making a GET. So at best this has debug value. Unless we encounter a partner that MUST have this implemented I would suggest to squarely ignore it.

[andacata]

I'm on the same point as you, ignoring it 😄

I hope no critical partner who relies on that wants to connect quickly 🤞