Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple open Requests that lead to a Relationship can exist for the same Identity #309

Conversation

jkoenig134
Copy link
Member

Readiness checklist

  • I added/updated tests.
  • I ensured that the PR title is good enough for the changelog.
  • I labeled the PR.

@jkoenig134 jkoenig134 added the bug Something isn't working label Oct 23, 2024
@jkoenig134
Copy link
Member Author

@stnmtz & @tnotheis maybe you also want to look over this.

Copy link

codecov bot commented Oct 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
packages/runtime/src/modules/RequestModule.ts 0.00% 23 Missing ⚠️
Files with missing lines Coverage Δ
packages/runtime/src/modules/RequestModule.ts 56.68% <0.00%> (-4.07%) ⬇️

Copy link
Contributor

@britsta britsta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized a while ago that some validation takes place in the modules, although it should actually take place at a deeper level. Otherwise, if no modules are used and the methods are called manually, strange things can happen. In the case of this validation, I would expect it to be included in the received method of the IncomingRequestsController.

@jkoenig134
Copy link
Member Author

@britsta all these validations (including the new one I've added) are checking things that are important for RequestModule logic.

To make this clearer using an example: the consumption library doesn’t care at all if there is a Relationship or another open Request because it doesn't decides how the response is sent out.

@britsta
Copy link
Contributor

britsta commented Oct 24, 2024

@britsta all these validations (including the new one I've added) are checking things that are important for RequestModule logic.

To make this clearer using an example: the consumption library doesn’t care at all if there is a Relationship or another open Request because it doesn't decides how the response is sent out.

No matter we have still validation in the modules or not, how can we ensure the correctness of the processes in case an integrator isn't using the modules? We still need the option to add the appropriate validation for this case.

@jkoenig134
Copy link
Member Author

We don't, but when the integrator doesn't uses the Modules there are many other problems.

@jkoenig134
Copy link
Member Author

how can we ensure the correctness of the processes in case an integrator isn't using the modules

Also - when the integrator doesn't use the modules, then he's not using our process and their process could be valid.

@britsta
Copy link
Contributor

britsta commented Oct 24, 2024

@jkoenig134 Mhh alright, let's discuss this again somewhere else. :D For the moment, the newly added validation can stay with the other validation in the Module. 👼

@britsta britsta dismissed their stale review October 24, 2024 10:05

Does not need to be discussed in this PR.

@jkoenig134
Copy link
Member Author

Thanks master 😅

@britsta
Copy link
Contributor

britsta commented Oct 24, 2024

Thanks master 😅

@jkoenig134 It's been a long time since anyone has referred to me as master of disaster 👀

@jkoenig134 jkoenig134 requested a review from britsta October 28, 2024 08:28
@jkoenig134 jkoenig134 requested a review from stnmtz October 28, 2024 12:07
@jkoenig134 jkoenig134 merged commit 6668350 into main Oct 28, 2024
18 checks passed
@jkoenig134 jkoenig134 deleted the multiple-open-Requests-that-lead-to-a-Relationship-can-exist-for-the-same-Identity branch October 28, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants