feat: support DSP correct state in deprovisioning #5040
Conversation
wolf4ood
added
enhancement
wolf4ood
force-pushed
the
feat/5032_complete_dsp_tck_integration
branch
2 times, most recently
from
870c7dc to
b32fedb
Compare
wolf4ood
requested review from
ndr-brt,
paullatzelsperger,
ronjaquensel and
juliapampus
as code owners
| @@ -143,6 +143,8 @@ public class TransferProcess extends StatefulEntity<TransferProcess> { | |||
| private String transferType; | |||
| private String dataPlaneId; | |||
|
|
|||
| private Integer previousState; | |||
There was a problem hiding this comment.
just a nit: for other state-related fields we use int instead of Integer, maybe worth to have this aligned
There was a problem hiding this comment.
Woulnd't be a more "holistic" approach to just keep memory of all the state changes? That will permit other logic based on the entity history to be evaluated in a better way (see the <action>WasRequestedByTheCounterParty), because a previousState field that's only set in particular occasions could be misunderstood easily.
|
We could discuss about naming, but I don't think memory will fit the purpose. Currently we have a the deprovisining states that are considered completed for DSP, which is wrong. For us deprovisioning is also considered a final state and we don't know how to map the deprovisioning in the correct DSP state unless we know which is the origination or the cause of the deprovisioning eg the transfer as either terminated or completed. |
My proposal wasn't just to rename the field, but to not to have a simple field that stores only (eventually) the previous state, instead to keep a record of the state changes, from which we can understand if the transfer has been in any case, theoretically provisioning/deprovisioning states should go away once #4793 is completed (I'm still working on it... slowly!) |
|
Then if they are going we could think about leaving those DSP failing tests as allowed for the time being if we do not want to dirty the DB just for this For the history of changes it would be ok for me but only in memory will not solve this use case and probably others. |
We need to pass the tests. I think we should put the simple fix in for now as we are under pressure to complete the TCK in the next couple of days. We can work on a loner-term solution later. |
|
Since it is a patch to have tests working, could we base the decision "has it been completed or terminated" on the value of the "errorDetail" variable? it should be set only with terminated transfers, that will save us a potentially unnecessary schema migration |
|
we could try but if I remember correctly the |
Yes, and that can vary by implementation. |
|
Also I tried this workaround, We'd have to clean the |
|
Here's the changeset with just the workaround main...wolf4ood:DataSpaceConnector:feat/5032_complete_dsp_tck_integration_workaround |
wolf4ood
force-pushed
the
feat/5032_complete_dsp_tck_integration
branch
from
b32fedb to
921b138
Compare
|
I've applied the changeset to this PR. Now it does not require a migration and It only check the |
wolf4ood
removed
breaking-change
What this PR changes/adds
Fixes dsp-tck tests:
TP:01-01TP:01-03TP:01-05We will base the decision if a TP is
TERMINATEDorCOMPLETEDonly by checking theerrorDetailfield as a temporary workaround. This will not require any changes for users.We can revisit this behavior once #4793 is in.
Why it does that
dsp-compliance
Further notes
List other areas of code that have changed but are not necessarily linked to the main feature. This could be method
signature changes, package declarations, bugs that were encountered and were fixed inline, etc.
Who will sponsor this feature?
Please @-mention the committer that will sponsor your feature.
Linked Issue(s)
Closes #5032
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.