#118: adding support for MultipleResultFunction with status for Doobie #120
Conversation
lsulak
requested review from
benedeki,
dk1844,
jakipatryk and
Zejnilovic
as code owners
JaCoCo code coverage report - scala 2.13.12
Files
|
JaCoCo code coverage report - scala 2.12.17
Files
|
lsulak
added
the
work in progress
…ypes on many places
…tions, unit tests and small refactoring
lsulak
removed
the
work in progress
…st one will be returned if multiple errors were present with the same max count
| case Left(statusException) => Left(statusException) | ||
| case Right(value) => Right(getResult(value)) | ||
| ): ExceptionOrStatusWithDataRow[R] = { | ||
| val o = checkStatus(statusWithData) |
There was a problem hiding this comment.
Consider using more descriptive names for variables in this method.
There was a problem hiding this comment.
Oh, damned! Apologies, this is a result of me debugging this and forgetting to change it back. Rookie mistake :D
| * It provides an implementation for aggregating error statuses of a function invocation into a single error | ||
| * by choosing the error that occurred the most. | ||
| */ | ||
| trait ByMajorityErrorsStatusAggregator extends StatusAggregator { |
There was a problem hiding this comment.
I don't know if this is needed. I can hardly imagine someone would want to aggregate errors in this way.
| override def aggregate[R](statusesWithData: Seq[ExceptionOrStatusWithDataRow[R]]): ExceptionOrStatusWithDataResultAgg[R] = { | ||
| val firstRow = statusesWithData.headOption | ||
|
|
||
| val dataFinal = gatherDataWithStatuses(statusesWithData) |
There was a problem hiding this comment.
We need to gather data only if first row didn't contain an error.
| trait ByFirstErrorStatusAggregator extends StatusAggregator { | ||
|
|
||
| override def aggregate[R](statusesWithData: Seq[ExceptionOrStatusWithDataRow[R]]): ExceptionOrStatusWithDataResultAgg[R] = { | ||
| val firstError = gatherExceptions(statusesWithData).headOption |
There was a problem hiding this comment.
True, I'll change this
benedeki
removed
the
dependent
|
This item depends on: |
…e/118-add-DoobieMultipleResultFunctionWithStatus
| * @param data the data of one row (barring the status fields) | ||
| * @tparam D the type of the data | ||
| */ | ||
| case class Row[D](functionStatus: FunctionStatus, data: D) |
There was a problem hiding this comment.
I am not sure if Row is a good name. Also why D for the type parameter?
There was a problem hiding this comment.
D simply says that it's the data, or, 'effective/actual data' (sometimes this term can be find in literature). So basically a row without the status-related columns.
Hmm, why not? Row indicates that 1 instance of this should represent 1 row being returned from the DB. I had the name much more complicated, and it's true that Row is pretty simple, naive even, but when I checked the usecases and thought about its internal structure, I think that it's good enough name for this (couldn't come up with anything better and I didn't want the names to be sentences :D )
Also, this naming was not chosen to be 'isolated' to this Row class - we wanted to make it kind of relatable and consistent with the rest of the case classes in this module - see the classes below in this module :)
But as always, I'm open to proposals
| * | ||
| * Note: D here represents a single row reduced by status-related columns, i.e. a type of data. | ||
| */ | ||
| type FailedOrRow[D] = Either[StatusException, Row[D]] |
There was a problem hiding this comment.
How about StatusExceptionOr[A] - it's more common to have sth like ErrorOr[A] or sth like my suggestion which has the type encoding the failed value in the name.
There was a problem hiding this comment.
Hmm but sometimes it can happen that the type parameter is provided somehow automatically (is the compiler can understand it from the caller, then I won't need to specify it explicitly), which would lead to odd name
On the other hand, we can always explicitly supply it..
I must admit that I haven't really seen such approach in the past, but then you are much more experienced in Scala - I'll think about it; but basically the main idea was to make those 3 classes relatable = so I value 3 classes named well enough and relate-able more, rather than each class perfectly named but not relate-able. WDYT?
| * | ||
| * Note: D here represents a single row reduced by status-related columns, i.e. a type of data. | ||
| */ | ||
| type FailedOrRowSet[D] = Either[StatusException, Seq[Row[D]]] |
There was a problem hiding this comment.
Same as above. I would reconsider the name.
There was a problem hiding this comment.
acknowledged, but see my answer above
There was a problem hiding this comment.
I would also consider name change, but little different.
FailedOrRows[D] or FailedOrRowSeq[D]? 🤔
There was a problem hiding this comment.
I like FailedOrRows, thanks
| functionNameOverride: Option[String] = None | ||
| )(implicit schema: DBSchema, dBEngine: E) | ||
| extends DBFunctionWithStatus[I, R, E, F](functionNameOverride) | ||
| with StatusAggregator { |
There was a problem hiding this comment.
I don't like the fact it requires StatusAggregator to be mixed-in. I believe this should be optional and the return value of DBMultipleResultFunctionWithStatus should be a sequence of Either (or its alias). The aggregation should be in my opinion applied optionally later on as one might be interested also in all statuses.
There was a problem hiding this comment.
yes that's basically the hardest 'design decision' in this PR I think. I appreciate / see the benefits of both approaches, that's why I was kind of undecided yet and 'waiting' for 3rd opinion :D
There was a problem hiding this comment.
Okay I though about it and:
- I think status aggregation in general is gonna be used quite common and is therefore very helpful - just need to provide implementation that is convenient for us and users
- I still really like and prefer mix-in approach - it's consistent and 'expected' from user point of view how things are plugged together, like nice reusable and changeable lego blocks
- I understand that having a mandatory agg implementation might be too much and sometimes not needed. Therefore I implemented it separately - one with mixin and the other without.
- With this 'new' approach, I created respective 'utils' ('helper' classes) to both, Doobie and Slick, and sufficiently covered it all with tests.
Does that influence your opinion on this status aggregation related feature?
…e/118-add-DoobieMultipleResultFunctionWithStatus
JaCoCo core code coverage report - scala 2.13.12
|
JaCoCo slick code coverage report - scala 2.13.12
|
JaCoCo doobie code coverage report - scala 2.13.12
|
| @@ -37,21 +37,22 @@ trait QueryWithStatus[A, B, R] { | |||
| * @param initialResult - the initial result of the query | |||
| * @return the status with data | |||
There was a problem hiding this comment.
| * @return the status with data | |
| * @return the data with status |
There was a problem hiding this comment.
sure, changed on all the places
| * @tparam A - the initial result type of the query (a row basically, having status-related columns as well) | ||
| * @tparam B - the intermediate result type of the query (a row without status columns, i.e. data only) |
There was a problem hiding this comment.
What about renaming these generics to:
DS - data with status
D - data
There was a problem hiding this comment.
adopted, I like it!
| * | ||
| * Note: D here represents a single row reduced by status-related columns, i.e. a type of data. | ||
| */ | ||
| type FailedOrRowSet[D] = Either[StatusException, Seq[Row[D]]] |
There was a problem hiding this comment.
I would also consider name change, but little different.
FailedOrRows[D] or FailedOrRowSeq[D]? 🤔
…g function util class separately so that users can choose - also available in Slick and Doobie, covered all by tests
lsulak
deleted the
feature/118-add-DoobieMultipleResultFunctionWithStatus
branch
What was done:
DBMultipleResultFunctionWithStatus,DBSingleResultFunctionWithStatusandDBOptionalResultFunctionWithStatusbased onDBFunctionWithStatusas it was for non-status classes, i.e. keeping the design consistent across the core moduleDoobieOptionalResultFunctionWithStatusandDoobieMultipleResultFunctionWithStatusSlickOptionalResultFunctionWithStatusandSlickMultipleResultFunctionWithStatusrunWithStatusand all dependant methods fromF[Either[StatusException, R]]toF[Either[StatusException, FunctionStatusWithData[R]]](and also their Seq-like versions)StatusAggregationwith three simple yet most likely used algorithms for aggregating error statuses, i.e. transformation fromF[Seq[Either[StatusException, FunctionStatusWithData[R]]]]toF[Either[StatusException, Seq[FunctionStatusWithData[R]]]]Depends on #119
Closes #118
Closes #115