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

Binder in JerseyRestService binds resource instances from other contexts #3312

Closed
pneuschwander opened this issue Jul 18, 2023 · 3 comments · Fixed by #3327
Closed

Binder in JerseyRestService binds resource instances from other contexts #3312

pneuschwander opened this issue Jul 18, 2023 · 3 comments · Fixed by #3327
Assignees
Labels
bug_report Suspected bugs, awaiting triage

Comments

@pneuschwander
Copy link

Bug Report

Describe the Bug

Binder in JerseyRestService binds resource instances from other contexts.

Expected Behavior

Binder in JerseyRestService binds resource instances only from the correct (single) context.

Observed Behavior

Binder in JerseyRestService binds resource instances from other contexts.

Steps to Reproduce

Context Information

Bug in EDC version 0.1.3 still present, observed with sovity edc community edition connector version 4.0.1 that is using EDC in a 0.0.1 version.

I noticed The /check/* endpoint has been moved under the 'management' context, please update your url accordingly, because this endpoint will be deleted in the next releases in the logs even though that they use the check endpoint on the management API in the docker image.

=> The wrong ObservabilityApiController instance (that one of the default context with the deprecated=true parameter) was bound to the management context/servlet.

Detailed Description

private class Binder extends AbstractBinder {

        @Override
        protected void configure() {
            controllers.forEach((key, value) -> value.forEach(c -> bind(c).to((Class<? super Object>) c.getClass())));
        }
    }

https://github.com/eclipse-edc/Connector/blob/main/extensions/common/http/jersey-core/src/main/java/org/eclipse/edc/web/jersey/JerseyRestService.java#L116

controllers is the Map<String, List> that holds the resources of all the contexts. The Key of the entries is the contextAlias.

The logic of the binder binds resources of any context but should only bind the resources of the correct, single context.

Possible Implementation

We could provide the resources/controllers of the context to the binder via ctor parameter in this line easily: https://github.com/eclipse-edc/Connector/blob/main/extensions/common/http/jersey-core/src/main/java/org/eclipse/edc/web/jersey/JerseyRestService.java#L91

as the resources/controllers are available as controllers in this scope. We could have an attribute List<Object> controllers in the Binder class and make it a static inner class as we would no longer use the controllers-Map from the outer class.

Or we could provide the contextAlias via ctor parameter to the Binder and change the implementation to only consider those entries of controllers.get(contextAlias).forEach in this line: https://github.com/eclipse-edc/Connector/blob/main/extensions/common/http/jersey-core/src/main/java/org/eclipse/edc/web/jersey/JerseyRestService.java#L116

@github-actions
Copy link

Thanks for your contribution 🔥 We will take a look asap 🚀

@efiege
Copy link

efiege commented Jul 18, 2023

Just for clarification: Version 4.0.1 does not include the latest changes of the core-EDC. It still uses the milestone-8-Extensions, as we are currently migrating to the 0.1.3 version.

@ndr-brt ndr-brt added bug_report Suspected bugs, awaiting triage triage all new issues awaiting classification and removed triage all new issues awaiting classification labels Jul 19, 2023
@ndr-brt ndr-brt self-assigned this Jul 24, 2023
@ndr-brt
Copy link
Member

ndr-brt commented Jul 24, 2023

thanks for reporting, PR is on the way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug_report Suspected bugs, awaiting triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants