lorenzodonini / ocpp-go

Open Charge Point Protocol implementation in Go
MIT License
277 stars 126 forks source link

Add AddHttpHandler #180

Closed shiv3 closed 1 year ago

shiv3 commented 1 year ago

We are running OCPP's CentralSystem in a container. Since only one port can be exported in a container, it is difficult to start an HTTP server and a websocket server at the same time to accept health checks, remoteTransaction, and so on. Therefore, we would like to add a function that can implement OCPP websocket and other handlers.

shiv3 commented 1 year ago

@lorenzodonini Thanks for the quick reply.

It may be an anti-pattern, but we are trying to run ocpp services on gcp cloudrun and it only supports one port.. We considered configuring remoteTransaction to run asynchronously using a datastore such as database or redis, but this would be complicated, so we decided to add a handler to the existing websocket http server. ( I don't have the details, but isn't #176 a similar requirement? 🤔

I chose to inject the handler because I thought that injecting the router would overwrite all handlers, making it difficult to know when to add an existing wsHandler.

shiv3 commented 1 year ago

Also, unrelated to this PR, gorilla/mux and gorilla/websocket is already out of support, do you have any plans to migrate to other router libraries such as chi or alexedwards/scs?

lorenzodonini commented 1 year ago

I don't have the details, but isn't https://github.com/lorenzodonini/ocpp-go/issues/176 a similar requirement?

Yes they are indeed related, but I'm seeing a challenge in combining your PR with #176. To make the ws package compatible with all stdlib-compatible HTTP packages (like Gin), we'd need to support either:

Both approaches come with different challenges.

I chose to inject the handler because I thought that injecting the router would overwrite all handlers, making it difficult to know when to add an existing wsHandler.

Injecting a router at creation time would actually be fine, since the handlers are added when the server is started. However, on second thought, this would directly expose the underlying gorilla package, which would be a downside. So your solution actually makes more sense 👍

I’ll merge your PR for now since it doesn’t introduce any breaking changes, but I might tackle the challenge mentioned above in a follow-up, if you don’t mind.

Also, unrelated to this PR, gorilla/mux and gorilla/websocket is already out of support, do you have any plans to migrate to other router libraries such as chi or alexedwards/scs?

The gorilla packages are very well tested and have been stable for several years now (they're used in production in lots of projects). I don't think it's urgent to address this atm. If the need ever arises, like new security vulnerabilities, I'm happy to swap them out for another package that is compatible with the stdlib.