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

Support for relative urls in batch payload #122 #804

Conversation

NilsEngelbach
Copy link

This pull request enables setting the BatchPayloadUriOption to support relative urls (#122).

var settings = new ODataClientSettings(httpClient, new Uri("odata/v1", UriKind.Relative))
{
    BatchPayloadUriOption = Microsoft.OData.BatchPayloadUriOption.RelativeUri
};

I will also provide unit tests soon.

Copy link
Member

@watfordgnf watfordgnf left a comment

Choose a reason for hiding this comment

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

Few grammatical nits, however, we'll need a test or two for this.

Thank you for the submission!

@NilsEngelbach
Copy link
Author

@watfordgnf Thank you for the fast feedback.

I added 2 unit tests but i am not sure if they are added in the right place. I also adjusted the Mockdata "by hand" is there a way to generate the proper request/response mock somehow automatically?
Should this be tested with IntegrationTests instead?

I also recognized that the feature will only be available for OData V4, should this be added in the comment for the new setting?

Copy link
Author

@NilsEngelbach NilsEngelbach left a comment

Choose a reason for hiding this comment

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

Applied all requested changes

@NilsEngelbach
Copy link
Author

@watfordgnf Can I do something more to complete this pull request?

@watfordgnf
Copy link
Member

@NilsEngelbach I haven't had an opportunity to pull this down locally and test it. I'm changing jobs in just over a week, so things are a bit busy :)

@object
Copy link
Member

object commented Apr 21, 2021

@NilsEngelbach I ran tests for your PR and there were more than 300 unit test failures, Obviously something went wrong. Please make sure all tests pass.

Since the change can be tested using integration tests, can you write at least one such test for TripPinTests (part of Simple.OData.CLient.IntegrationTests)? And since this is OData 4 only change, perhaps it's worth to mention in XML comments for new ODataClientSettings.

@NilsEngelbach
Copy link
Author

@watfordgnf no worries and all the best for the new job, I was just surprised because your first response was so quick :)

@object I reverted my unit tests which were causing trouble with the other unit tests and added TripPin Integrationtests.

But it looks like the services does not support relative uris (the batch reader is missing the base uri) and returns the error:

{
  "error":{
    "code": "InternalServerError",
    "message": "The relative URI 'Airlines' was specified in a batch operation, but a base URI was not specified for the batch writer or batch reader.",
    "innererror":{
      "message": "The relative URI 'Airlines' was specified in a batch operation, but a base URI was not specified for the batch writer or batch reader.",
      "type": "Microsoft.OData.Core.ODataException",
      "stacktrace": "at Microsoft.OData.Core.ODataBatchUtils.CreateOperationRequestUri(Uri uri, Uri baseUri, IODataUrlResolver urlResolver)
          at Microsoft.OData.Core.ODataBatchReader.ParseRequestLine(String requestLine, String& httpMethod, Uri& requestUri)
          at Microsoft.OData.Core.ODataBatchReader.CreateOperationRequestMessageImplementation()
          at Microsoft.OData.Core.ODataBatchReader.InterceptException[T](Func`1 action)
          at Microsoft.OData.Core.ODataBatchReader.CreateOperationRequestMessage()
          at Microsoft.Test.OData.Services.ODataWCFService.Handlers.BatchHandler.Process(IODataRequestMessage requestMessage, IODataResponseMessage responseMessage)
          at Microsoft.Test.OData.Services.ODataWCFService.Handlers.RequestHandler.Process(Stream requestStream)
          at Microsoft.Test.OData.Services.ODataWCFService.Handlers.RequestHandler.Process(Stream requestStream)
          at Microsoft.Test.OData.Services.ODataWCFService.Handlers.RootRequestHandler.Process(Stream requestStream)"
    }
  }
}

Who operates/maintains the TripPin Service?

I checked the request messages from the integration tests and they look correctly.
I also tried the new setting with my own OData service and it is working as expected...

@object
Copy link
Member

object commented Apr 21, 2021

@watfordgnf no worries and all the best for the new job, I was just surprised because your first response was so quick :)

@object I reverted my unit tests which were causing trouble with the other unit tests and added TripPin Integrationtests.

But it looks like the services does not support relative uris (the batch reader is missing the base uri) and returns the error:

{
  "error":{
    "code": "InternalServerError",
    "message": "The relative URI 'Airlines' was specified in a batch operation, but a base URI was not specified for the batch writer or batch reader.",
    "innererror":{
      "message": "The relative URI 'Airlines' was specified in a batch operation, but a base URI was not specified for the batch writer or batch reader.",
      "type": "Microsoft.OData.Core.ODataException",
      "stacktrace": "at Microsoft.OData.Core.ODataBatchUtils.CreateOperationRequestUri(Uri uri, Uri baseUri, IODataUrlResolver urlResolver)
          at Microsoft.OData.Core.ODataBatchReader.ParseRequestLine(String requestLine, String& httpMethod, Uri& requestUri)
          at Microsoft.OData.Core.ODataBatchReader.CreateOperationRequestMessageImplementation()
          at Microsoft.OData.Core.ODataBatchReader.InterceptException[T](Func`1 action)
          at Microsoft.OData.Core.ODataBatchReader.CreateOperationRequestMessage()
          at Microsoft.Test.OData.Services.ODataWCFService.Handlers.BatchHandler.Process(IODataRequestMessage requestMessage, IODataResponseMessage responseMessage)
          at Microsoft.Test.OData.Services.ODataWCFService.Handlers.RequestHandler.Process(Stream requestStream)
          at Microsoft.Test.OData.Services.ODataWCFService.Handlers.RequestHandler.Process(Stream requestStream)
          at Microsoft.Test.OData.Services.ODataWCFService.Handlers.RootRequestHandler.Process(Stream requestStream)"
    }
  }
}

Who operates/maintains the TripPin Service?

I checked the request messages from the integration tests and they look correctly.
I also tried the new setting with my own OData service and it is working as expected...

That's sad. TrinPinSerice is built and maintained by Microsoft. I suggest you keep the test but mark it as ignored, so it can be activated it they update the service implementation.

@NilsEngelbach
Copy link
Author

I hosted the TripPinService (https://github.com/OData/ODataSamples/tree/master/Scenarios/TripPin/src/webapi) locally and the Integrationtests are working with this local service.

I created an issue OData/ODataSamples#140 and disabled the failing tests.

@object object merged commit bd297a7 into simple-odata-client:master Apr 23, 2021
@object
Copy link
Member

object commented Apr 23, 2021

Thank you for the PR. Merged and released as version 5.21.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants