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.

Sign up for GitHub

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

Jump to bottom

fix: stop method property from being mandatory for push http transfers #3797

Merged
merged 2 commits into from
Jan 25, 2024
Merged

fix: stop method property from being mandatory for push http transfers #3797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3455c30
fix: stop method prop from being mandatory for push http transfers
ndr-brt Jan 24, 2024
681b84f
dependencies
ndr-brt Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 2 additions & 5 deletions DEPENDENCIES
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,8 @@ maven/mavencentral/javax.ws.rs/javax.ws.rs-api/2.1, (CDDL-1.1 OR GPL-2.0 WITH Cl
maven/mavencentral/joda-time/joda-time/2.10.5, Apache-2.0, approved, clearlydefined
maven/mavencentral/junit/junit/4.13.2, EPL-2.0, approved, CQ23636
maven/mavencentral/net.bytebuddy/byte-buddy-agent/1.14.1, Apache-2.0, approved, #7164
maven/mavencentral/net.bytebuddy/byte-buddy-agent/1.14.10, Apache-2.0, approved, #7164
maven/mavencentral/net.bytebuddy/byte-buddy/1.12.21, Apache-2.0 AND BSD-3-Clause, approved, #1811
maven/mavencentral/net.bytebuddy/byte-buddy-agent/1.14.11, Apache-2.0, approved, #7164
maven/mavencentral/net.bytebuddy/byte-buddy/1.14.1, Apache-2.0 AND BSD-3-Clause, approved, #7163
maven/mavencentral/net.bytebuddy/byte-buddy/1.14.10, Apache-2.0 AND BSD-3-Clause, approved, #7163
maven/mavencentral/net.bytebuddy/byte-buddy/1.14.11, Apache-2.0 AND BSD-3-Clause, approved, #7163
maven/mavencentral/net.java.dev.jna/jna/5.13.0, Apache-2.0 AND LGPL-2.1-or-later, approved, #6709
maven/mavencentral/net.javacrumbs.json-unit/json-unit-core/2.36.0, Apache-2.0, approved, clearlydefined
Expand Down Expand Up @@ -211,7 +209,6 @@ maven/mavencentral/org.apache.velocity.tools/velocity-tools-generic/3.1, Apache-
maven/mavencentral/org.apache.velocity/velocity-engine-core/2.3, Apache-2.0, approved, #2478
maven/mavencentral/org.apache.velocity/velocity-engine-scripting/2.3, Apache-2.0, approved, clearlydefined
maven/mavencentral/org.apiguardian/apiguardian-api/1.1.2, Apache-2.0, approved, clearlydefined
maven/mavencentral/org.assertj/assertj-core/3.24.2, Apache-2.0, approved, #6161
maven/mavencentral/org.assertj/assertj-core/3.25.1, Apache-2.0, approved, #12585
maven/mavencentral/org.awaitility/awaitility/4.2.0, Apache-2.0, approved, clearlydefined
maven/mavencentral/org.bouncycastle/bcpkix-jdk18on/1.72, MIT, approved, #3789
Expand Down Expand Up @@ -298,7 +295,7 @@ maven/mavencentral/org.mock-server/mockserver-client-java/5.15.0, Apache-2.0 AND
maven/mavencentral/org.mock-server/mockserver-core/5.15.0, Apache-2.0, approved, clearlydefined
maven/mavencentral/org.mock-server/mockserver-netty/5.15.0, Apache-2.0, approved, #9276
maven/mavencentral/org.mockito/mockito-core/5.2.0, MIT AND (Apache-2.0 AND MIT) AND Apache-2.0, approved, #7401
maven/mavencentral/org.mockito/mockito-core/5.8.0, MIT AND (Apache-2.0 AND MIT) AND Apache-2.0, approved, #11787
maven/mavencentral/org.mockito/mockito-core/5.9.0, MIT AND (Apache-2.0 AND MIT) AND Apache-2.0, approved, #12774
maven/mavencentral/org.mockito/mockito-inline/5.2.0, MIT, approved, clearlydefined
maven/mavencentral/org.mozilla/rhino/1.7.7.2, MPL-2.0 AND BSD-3-Clause AND ISC, approved, CQ16320
maven/mavencentral/org.objenesis/objenesis/3.3, Apache-2.0, approved, clearlydefined
Expand Down
2 changes: 1 addition & 1 deletion ...eclipse/edc/connector/dataplane/http/params/decorators/BaseSourceHttpParamsDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public HttpRequestParams.Builder decorate(DataFlowRequest request, HttpDataAddre
}

private @NotNull String extractMethod(HttpDataAddress address, DataFlowRequest request) {
if (Boolean.parseBoolean(address.getProxyMethod())) {
if (Boolean.parseBoolean(address.getProxyMethod()) && "HttpProxy".equals(request.getDestinationDataAddress().getType())) {
Copy link

@efiege efiege Jan 24, 2024 •
edited

Choose a reason for hiding this comment

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

I don't expect that this will solve the issue regarding the HttpPush. This will disable the feature of proxying http methods for the HttpPush, as now just "GET" requests will be sent to the providing backend.

Our expectation would be that http-methods also could be proxied for the HttpPush. We could also help providing this fix, if this is more convenient.

Copy link
Member Author

@ndr-brt ndr-brt Jan 24, 2024 •
edited

Choose a reason for hiding this comment

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

simply, for http push this condition won't ever met, and it will pass through line 60, so the method will be taken from the DataAddress (as it already does) if it's defined, otherwise it will use the default GET.

To verify that, just try to remove the proxyMethod property and set method instead on the source data address.

The proxy was made to work with pull, not for push

Copy link

@efiege efiege Jan 24, 2024 •
edited

Choose a reason for hiding this comment

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

simply, for http push this condition won't ever met, and it will pass through line 60, so the method will be taken from the DataAddress (as it already does) if it's defined, otherwise it will use the default GET.

Ohh yes, I have overlooked this: The HttpPush will use the method from the DataAddress.

The proxy was made to work with pull, not for push

This would implicate that certain HttpData assets can just be used for HttpProxy transfer-types and not for HttpData transfer-types. However it would make sense to also support this for the HttpPush.

So basically this would be a feature request?

Copy link
Member Author

Choose a reason for hiding this comment

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

theoretically yes, I'm not sure which would be the applications for that: the consumer should be the one who chose how the call to its service will be made.

Copy link

Choose a reason for hiding this comment

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

The applications would be the same as for the http-pull I think.

So for the http-pull the consumer is able to select the preferred http-method, which is used to access the provider system. This is enabled by using the prxoyMethod=true property in the asset of the provider.

For the http-push the transfer is also started by the consumer. Like for the http-pull the consumer should be able to select the prefered http-method, which is used to access the provider system.

Copy link
Member Author

Choose a reason for hiding this comment

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

in any case, also without the proxyMethod flag to true, the consumer can choose the method, that by default is GET. Really, all the proxyXXX stuff is there for the proxy use case ("nomen omen").

Copy link

Choose a reason for hiding this comment

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

Hey @ndr-brt,

I'm sry for coming back on this so lately, but this change is really important to us and I may not have understood this correctly.

After giving this a second look, I am also convinced, that this change won't affect passing methods for the HttpPush from the consumer.

My problem was, that I though the HttpDataAddress address parameter is always populated by the SourceDataAddress:

Connector/extensions/data-plane/data-plane-http/src/main/java/org/eclipse/edc/connector/dataplane/http/params/decorators/BaseSourceHttpParamsDecorator.java

Lines 55 to 61 in cf629c9

private @NotNull String extractMethod(HttpDataAddress address, DataFlowStartMessage request) {
if (Boolean.parseBoolean(address.getProxyMethod()) && "HttpProxy".equals(request.getDestinationDataAddress().getType())) {
return Optional.ofNullable(request.getProperties().get(METHOD))
.orElseThrow(() -> new EdcException(format("DataFlowRequest %s: 'method' property is missing", request.getId())));
}
return Optional.ofNullable(address.getMethod()).orElse(DEFAULT_METHOD);
}

but it turns out it also gets called for the DestinationDataAddress:

Connector/extensions/data-plane/data-plane-http/src/main/java/org/eclipse/edc/connector/dataplane/http/params/HttpRequestParamsProviderImpl.java

Lines 54 to 68 in cf629c9

@Override
public HttpRequestParams provideSourceParams(DataFlowStartMessage request) {
var params = HttpRequestParams.Builder.newInstance();
var address = HttpDataAddress.Builder.newInstance().copyFrom(request.getSourceDataAddress()).build();
sourceDecorators.forEach(decorator -> decorator.decorate(request, address, params));
return params.build();
}
@Override
public HttpRequestParams provideSinkParams(DataFlowStartMessage request) {
var params = HttpRequestParams.Builder.newInstance();
var address = HttpDataAddress.Builder.newInstance().copyFrom(request.getDestinationDataAddress()).build();
sinkDecorators.forEach(decorator -> decorator.decorate(request, address, params));
return params.build();
}

So indeed the extractMethod returns the method specified by the DestinationDataAddress.

Did I understood this correctly?

Thanks for fixing this!

return Optional.ofNullable(request.getProperties().get(METHOD))
.orElseThrow(() -> new EdcException(format("DataFlowRequest %s: 'method' property is missing", request.getId())));
}
Expand Down
39 changes: 31 additions & 8 deletions .../eclipse/edc/connector/dataplane/http/params/HttpRequestParamsProviderImplSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import org.eclipse.edc.connector.dataplane.http.spi.HttpDataAddress;
import org.eclipse.edc.connector.dataplane.http.spi.HttpRequestParamsProvider;
import org.eclipse.edc.spi.EdcException;
import org.eclipse.edc.spi.types.TypeManager;
import org.eclipse.edc.spi.types.domain.DataAddress;
import org.eclipse.edc.spi.types.domain.transfer.DataFlowRequest;
import org.junit.jupiter.api.Test;

Expand All @@ -31,10 +31,11 @@
import static org.eclipse.edc.connector.dataplane.spi.schema.DataFlowRequestSchema.METHOD;
import static org.eclipse.edc.connector.dataplane.spi.schema.DataFlowRequestSchema.PATH;
import static org.eclipse.edc.connector.dataplane.spi.schema.DataFlowRequestSchema.QUERY_PARAMS;
import static org.mockito.Mockito.mock;

class HttpRequestParamsProviderImplSourceTest {

private final HttpRequestParamsProvider provider = new HttpRequestParamsProviderImpl(null, new TypeManager());
private final HttpRequestParamsProvider provider = new HttpRequestParamsProviderImpl(mock(), mock());

@Test
void shouldMapDataFlowRequestToHttpRequest() {
Expand All @@ -49,7 +50,7 @@ void shouldMapDataFlowRequestToHttpRequest() {
var dataFlowRequest = DataFlowRequest.Builder.newInstance()
.processId(UUID.randomUUID().toString())
.sourceDataAddress(source)
.destinationDataAddress(dummyAddress())
.destinationDataAddress(dummyHttpDataAddress())
.build();

var params = provider.provideSourceParams(dataFlowRequest);
Expand Down Expand Up @@ -78,7 +79,7 @@ void shouldMapDataFlowRequestToHttpRequest_proxyDataFlowRequest() {
var dataFlowRequest = DataFlowRequest.Builder.newInstance()
.processId(UUID.randomUUID().toString())
.sourceDataAddress(source)
.destinationDataAddress(dummyAddress())
.destinationDataAddress(DataAddress.Builder.newInstance().type("HttpProxy").build())
.properties(Map.of(
METHOD, "proxy-method",
PATH, "proxy-path",
Expand Down Expand Up @@ -107,7 +108,7 @@ void shouldMapDataFlowRequestToHttpRequest_withDefaultValues() {
var dataFlowRequest = DataFlowRequest.Builder.newInstance()
.processId(UUID.randomUUID().toString())
.sourceDataAddress(source)
.destinationDataAddress(dummyAddress())
.destinationDataAddress(dummyHttpDataAddress())
.build();

var params = provider.provideSourceParams(dataFlowRequest);
Expand All @@ -121,7 +122,7 @@ void shouldMapDataFlowRequestToHttpRequest_withDefaultValues() {
}

@Test
void shouldThrowExceptionWhenProxyMethodIsMissing() {
void shouldThrowException_whenProxyMethodIsMissingAndDestinationIsHttpProxy() {
var source = HttpDataAddress.Builder.newInstance()
.baseUrl("http://source")
.proxyMethod("true")
Expand All @@ -133,13 +134,35 @@ void shouldThrowExceptionWhenProxyMethodIsMissing() {
var dataFlowRequest = DataFlowRequest.Builder.newInstance()
.processId(UUID.randomUUID().toString())
.sourceDataAddress(source)
.destinationDataAddress(dummyAddress())
.destinationDataAddress(DataAddress.Builder.newInstance().type("HttpProxy").build())
.build();

assertThatExceptionOfType(EdcException.class).isThrownBy(() -> provider.provideSourceParams(dataFlowRequest));
}

private HttpDataAddress dummyAddress() {
@Test
void shouldUseSourceMethod_whenProxyMethodIsMissingAndDestinationIsNotHttpProxy() {
var source = HttpDataAddress.Builder.newInstance()
.baseUrl("http://source")
.proxyMethod("true")
.path("test-path")
.queryParams("foo=bar")
.contentType("test/content-type")
.nonChunkedTransfer(true)
.method("POST")
.build();
var dataFlowRequest = DataFlowRequest.Builder.newInstance()
.processId(UUID.randomUUID().toString())
.sourceDataAddress(source)
.destinationDataAddress(dummyHttpDataAddress())
.build();

var params = provider.provideSourceParams(dataFlowRequest);

assertThat(params.getMethod()).isEqualTo("POST");
}

private HttpDataAddress dummyHttpDataAddress() {
return HttpDataAddress.Builder.newInstance().baseUrl("http://dummy").build();
}
}