service: update error types to be more logicless, simplify validations #402
Conversation
ilgooz
force-pushed
the
feature/service-errors-and-validations
branch
5 times, most recently
from
73400cf
to
8c469fd
Compare
…idations. closes #350, related with #348, a part of #337 * error types now doesn't call methods from its struct fields for validation. * remove isValid and Validate methods from types and adapt generic service.ValidateParametersSchema() method. * TestValidParameters removed from service package. no longer needed.
…d error checking logic to parent caller * TestCreateNotPresentEvent removed from event package, it has an equivalent TestEmitWrongEvent in interface/grpc/service. * TestCreateInvalidData removed from event package, an equivalent TestEmitInvalidData added to interface/grpc/service. * TestCreateInvalidTask removed from execution package, it has an equivalent TestExecuteWithInvalidTask in interface/grpc/core. * TestCompleteNotFound removed from execution package, an equivalent TestSubmitWithNonExistentOutputKey added to interface/grpc/service. * TestCreateInvalidInputs removed from execution package, an equivalent TestExecuteWithInvalidTaskInput added to interface/grpc/core. * TestCompleteInvalidOutputs removed from execution package, an equivalent TestSubmitWithInvalidTaskOutputs added to interface/grpc/service.
ilgooz
force-pushed
the
feature/service-errors-and-validations
branch
from
68b8e87
to
d2edf0d
Compare
ilgooz
requested review from
antho1404,
NicolasMahe and
krhubert
and removed request for
antho1404 and
NicolasMahe
api/execute_executor.go
Outdated
| } | ||
| } | ||
|
|
||
| exc, err := execution.Create(s, taskKey, taskInputs, tags) |
There was a problem hiding this comment.
I'm curious to have some arguments to move the validation outside of the create. For me the validation here is really related to the execution so it make sense that the execution package does the validation. Same point for the event and the results
There was a problem hiding this comment.
I can't 100% agree with this. calling methods of specialized packages like execution with invalid inputs seemed to me a bit messy. I believe that these validations should be done in the api package, a place that where we compose lots of types and different internal packages. This compositions requires some validations and I think doing this in package api feels natural. For Create(), to me, there is no difference between checking if the given service is nil -which doesn't make sense- and checking the existence of task.
But I'm not strongly against for your version, @NicolasMahe what do you think about this?
There was a problem hiding this comment.
I agree with Anthony, the validation make more sense in the Create function.
There was a problem hiding this comment.
Also, I thought you will put the validate functions in the service package and they returns directly the error, like in #348 (comment). Did you plan to do it in another PR?
There was a problem hiding this comment.
I'll do it in another PR
…rent caller, re-add removed tests
ilgooz
force-pushed
the
feature/service-errors-and-validations
branch
2 times, most recently
from
46ebd3c
to
a6cd6da
Compare
ilgooz
force-pushed
the
feature/service-errors-and-validations
branch
from
a6cd6da
to
565e17a
Compare
|
ServiceName field added to Invalid* error types in service package. |
depends on #383, it should be merged before this one. this PR will be rebased from dev after depended PR makes its way through dev.
please review this PR by commits.