search

anz-bank / sysl-go

Communication library used by SYSL-generated code written in Go.
Apache License 2.0
10 stars 14 forks source link

Export DB connection in DB Client so it can be used #176

Closed orlade-anz closed 4 years ago

orlade-anz commented 4 years ago

AFAICT, the purpose of a Client is to provide the means to make downstead requests. Currently, services that have a downstream dependency on a database receive a Client with a private (non-exported) connection. This means that because the user of the Client (custom code) is in a different package to the Client (generated code), the connection is inaccessible and the Client is useless.

IIUC an ideal solution would be to specify in Sysl the queries that need to be performed on the DB, and expose the means to execute those queries as exported functions, without exposing the underlying connection. However there will be cases where a DB is needed, but the set of necessary queries is not specified, and in that case the connection itself must be exposed for the querying business logic to be implemented.

orlade-anz commented 4 years ago

conn is used as internal property in the package.

That seems like a design bug.

Using conn is possible because GetCompanyLocationList is declared in the same package as GetCompanyLocationListClient: dbendpoints. in fact GetCompanyLocationList is only ever implemented in a dbendpoints test. I'm making the argument that it's not a good example.

If the impl that uses the client must be in the same package as the client, then you're preventing clients from having anything private; any internals that they need to do their job are effectively public. A workaround would be to have as fields only funcs that close over private state... but then you're fighting against the language to make the client provide a public API.

I would argue all implementation code should be in different packages to all generated/framework code to reduce the risk of leaking abstractions from private state. As such all pieces of a Client struct that are expected to be used in implementation code should be public. As per the description, I don't think the sql.Conn should be exported in general; there should be exported functions (or prepared statements) that use the conn internally to execute queries per the spec. However in situations where there are no specified queries (or an incomplete set), the implementation code should be provided with a sql.Conn to do the needful.

orlade-anz commented 4 years ago

@andrewemeryanz for a sysl-go perspective.

anzdaddy commented 4 years ago

The generated code that populates conn and the developer-written code that uses it must be in different directories/packages. Therefore, access to conn must be via exports for at least one side.

orlade-anz commented 4 years ago

The generated servicehandler.go is given a DB describing the downstream interface. It get a conn from that, and creates the client with it. Both the declaration of the field and the creation where the field is set are in client.arrai, and svc_handler.arrai uses client.new to generate the creation code to inject into the handler. It's a bit trippy, kinda half imperative, half object oriented.

orlade-anz commented 4 years ago

Check seems to fail because it's using the old nodiff that I fixed... not sure why it's stale.