Fixes #9701: Store data sources in backend #1401
Conversation
|
PR rebased |
VinceMacBuche
force-pushed
the
ust_9701/store_data_sources_in_backend
branch
from
5a946bb
to
600fbe1
Compare
|
PR rebased |
VinceMacBuche
force-pushed
the
ust_9701/store_data_sources_in_backend
branch
2 times, most recently
from
be179a9
to
2bc99af
Compare
| -- data source properties and status are valid json, until we can use postgres 9.2 keep text type (more details in configuration details) | ||
|
|
||
| , properties text | ||
| , status text |
There was a problem hiding this comment.
I believe they must not be empty, else it can't be changed to json latter on. So, nullable, but with a constrain for empty string
There was a problem hiding this comment.
Ok, I didn't know about that constraint, You added the constraint on configuration i'll keep the same
|
Nice! |
|
It will need the added getAllDataSources (without statuse). And I'm not a fan a using lift auto sterilization, but it's good enough for now. A little test to check update of status of only one node and it can be merge. |
|
This PR is not finished, lift serialization/unserialization does not work ( does not serialize datetime, can't unserialize datasource type without typehints ...) I suggest we use the same serialization than Rest API for now, Even if we know that they can change and drift in the future |
|
(I forgot to rudder-dev wip ... :( ) |
VinceMacBuche
force-pushed
the
ust_9701/store_data_sources_in_backend
branch
2 times, most recently
from
8bd3b96
to
4e44466
Compare
|
PR rebased |
VinceMacBuche
force-pushed
the
ust_9701/store_data_sources_in_backend
branch
from
4e44466
to
617aec5
Compare
|
Commit modified |
VinceMacBuche
force-pushed
the
ust_9701/store_data_sources_in_backend
branch
from
617aec5
to
db2b577
Compare
|
ps: it works, need more testing though |
|
And maybe some names are poor choices |
| @@ -275,5 +281,67 @@ class DataSourceRepoImpl( | |||
|
|
|||
| } | |||
|
|
|||
| class PGDataSourceRepository( | |||
There was a problem hiding this comment.
Could you use DataSourceJdbcRepository to match other repo naming scheme?
| sql.attempt.transact(xa).run match { | ||
| case \/-(x) => Full(source) | ||
| case -\/(ex) => Failure(s"could not update lastId from database, cause is: ${ex.getMessage}", Full(ex), Empty) | ||
| } |
There was a problem hiding this comment.
There is an auto transform to avoid that boilerplate, you could use:
sql.map(_ => source).attempt.transact(xa).run
The implicit def is in Doobie object (xorToBox)
| _ <- Update[DataSource](insert).run(source).attempt.transact(xa).run | ||
| } yield { | ||
| source | ||
| })*/ |
| val query = sql"""delete from datasources where id = ${sourceId}""" | ||
| for { | ||
| source <- get(sourceId).flatMap { _ match { | ||
| case None => Failure("no source") |
There was a problem hiding this comment.
If I understand correctly, here, trying to delete a source which doesn't exists will lead to an error. I think it should not be the case so that we have an idempotent operation for deletion (ie: deleting a non existing datasource is a success with invariant: "after deletion, the datasource does not exist"
| Composite[(DataSourceId,String)].xmap( | ||
| tuple => extractDataSource(tuple._1,parse(tuple._2)) match { | ||
| case Full(s) => s | ||
| case eb => throw new RuntimeException("oops") |
There was a problem hiding this comment.
Could you change the message to log what is the cause of the error ("error when deserialising blabla") and perhaps even the faulty json (not sure if it is already logged in extractDataSource)
VinceMacBuche
force-pushed
the
ust_9701/store_data_sources_in_backend
branch
from
db2b577
to
bc94802
Compare
|
Commit modified |
|
PR rebased |
VinceMacBuche
force-pushed
the
ust_9701/store_data_sources_in_backend
branch
from
bc94802
to
4385858
Compare
|
OK, merging this PR |
https://www.rudder-project.org/redmine/issues/9701
This PR consists in two things:
Adding a backend for datasources, easy part
Having one json serialization that will work both for backend and rest api. Most complicated part was that it has not the same needs (backend data must be complete, rest API may be partial)
I used some Monad magic from Scalaz (but I think i need an implicit somewere to make it much more readable)
Two implementation on json serialization, one with monad Id (backend, fails if a data is missing), one with Option (allow to have partial data)
enjoy!