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

docs: decision record about multiple protocol versions #4495

Merged

Conversation

wolf4ood
Copy link
Contributor

What this PR changes/adds

Decision record about multiple protocol versions

Why it does that

Briefly state why the change was necessary.

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.

Linked Issue(s)

Closes # <-- insert Issue number if one exists

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@wolf4ood wolf4ood self-assigned this Sep 25, 2024
@wolf4ood wolf4ood added the documentation Improvements or additions to documentation label Sep 25, 2024
@wolf4ood wolf4ood force-pushed the docs/dr_multiple_protocol_versions branch 2 times, most recently from a3d38c6 to 841a17e Compare September 26, 2024 07:40
@wolf4ood wolf4ood marked this pull request as ready for review September 26, 2024 15:55
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

just an idea... why not use the protocol field that already exist?
currently the value is always dataspace-protocol-http, it could become dataspace-protocol-http:version, that will likely simplify the work:

  • no need to change CN and TP entities
  • no need to change RemoteMessage interface and relative implementations
  • no need to change ContractRequest and TransferRequest

all the rest looks good

wdyt?

@wolf4ood
Copy link
Contributor Author

I also though about this, it will probably reduce the work for supporting an additional fields everywhere.
It make sense but it would require to register multiple DspHttpRemoteMessageDispatcher for different versions.
which has a downside that today we inject DspHttpRemoteMessageDispatcher and register all the message handling there

@wolf4ood
Copy link
Contributor Author

Probably by refactoring the RemoteMessageDispatcherRegistry

@ExtensionPoint
public interface RemoteMessageDispatcherRegistry {

    /**
     * Registers a dispatcher.
     */
    void register(String protocol, RemoteMessageDispatcher dispatcher);
}

Could allow us to register the same DspHttpRemoteMessageDispatcher for different versions

@ndr-brt
Copy link
Member

ndr-brt commented Sep 27, 2024

I also though about this, it will probably reduce the work for supporting an additional fields everywhere. It make sense but it would require to register multiple DspHttpRemoteMessageDispatcher for different versions. which has a downside that today we inject DspHttpRemoteMessageDispatcher and register all the message handling there

we could refactor the dispatcher registry to being able to support the version (e.g. by split the protocol field by : and detect the "name" part) so we can keep a single dispatcher implementation that will be able to discern between different versions and act properly.

otherwise also refactor the DspHttpRemoteMessageDispatcher to keep the common logic separated and make only the "delta" logic injectable so we can easily register multiple dispatcher for multiple versions with different injections/configurations.

Maybe we are getting too technical here, but it's just a matter of refactoring

@wolf4ood
Copy link
Contributor Author

wolf4ood commented Sep 27, 2024

i would try to stick to a single DspHttpRemoteMessageDispatcher since basically the diff is only the transformers and the json-ld context which can be configured in different context/scope per version. the logic then is the same.

Having automatically split in the registry will work but it will always dispatch even of unsupported versions. In case we could put a check in the implementation underlying

What about the refactoring I proposed above? in case i can amend the DR :) and go with the version in the protocol field

@ndr-brt
Copy link
Member

ndr-brt commented Sep 27, 2024

i would try to stick to a single DspHttpRemoteMessageDispatcher since basically the diff is only the transformers and the json-ld context which can be configured in different context/scope per version. the logic then is the same.

Having automatically split in the registry will work but it will always dispatch even of unsupported versions. In case we could put a check in the implementation underlying

What about the refactoring I proposed above? in case i can amend the DR :) and go with the version in the protocol field

looks good to me 👍

@wolf4ood wolf4ood force-pushed the docs/dr_multiple_protocol_versions branch from 841a17e to 4e58fb2 Compare September 27, 2024 09:53
@wolf4ood wolf4ood merged commit 067ded3 into eclipse-edc:main Sep 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants