Skip to content

Fix to defer resolution of ContentID in Uri - #481

Closed
gathogojr wants to merge 3 commits into
OData:mainfrom
gathogojr:fix/365-invalid-uri-in-batch-request-changeset
Closed

Fix to defer resolution of ContentID in Uri#481
gathogojr wants to merge 3 commits into
OData:mainfrom
gathogojr:fix/365-invalid-uri-in-batch-request-changeset

Conversation

@gathogojr

Copy link
Copy Markdown
Contributor

Fixes #365

This PR fixes the issue by deferring resolution of ContentID in Uri (e.g. $1/TestBs/$ref) to much later when the dependent operations have already been executed and the ContentID mapped to a location for a resource. That will be done here:

if (contentIdToLocationMapping != null)
{
string encodedUrl = context.Request.GetEncodedUrl();
string resolvedRequestUrl = ContentIdHelpers.ResolveContentId(encodedUrl, contentIdToLocationMapping);
Uri resolvedUri;
if (!string.IsNullOrEmpty(resolvedRequestUrl)
&& Uri.TryCreate(resolvedRequestUrl, UriKind.Absolute, out resolvedUri))
{
context.Request.CopyAbsoluteUrl(resolvedUri);
}
context.Request.SetODataContentIdMapping(contentIdToLocationMapping);
}

CopyAbsoluteUrl(Uri) is called and the request Url has been resolved

@mikepizzo

Copy link
Copy Markdown
Contributor

Do you have an added test that validates that the correct URL is executed (with $1 replaced by the locationheader returned by the request with id = 1)? When I tried a similar fix I got back a 404 for the second request because $1/TestBs/$ref was invalid (the $1 was never resolved).

@gathogojr

gathogojr commented Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

Do you have an added test that validates that the correct URL is executed (with $1 replaced by the locationheader returned by the request with id = 1)? When I tried a similar fix I got back a 404 for the second request because $1/TestBs/$ref was invalid (the $1 was never resolved).

@mikepizzo I applied the fix to the project I used to repro the issue and it works perfectly. Here's the batch payload I'm basing the test on and the CreateRef controller action is being hit. The $1 in the URL of the 3rd sub request and the $2 in the body are replaced successfully. I might need to understand what scenario is failing so I can try it out

--batch_c6718d3f-c42b-4650-bbf2-acf33e38c65d
Content-Type: multipart/mixed; boundary=changeset_cd219c83-deef-4b21-a6cf-59f0b8d960e3

--changeset_cd219c83-deef-4b21-a6cf-59f0b8d960e3
Content-Type: application/http
Content-Transfer-Encoding: binary
Content-ID: 1

POST http://localhost/odata/TestAs HTTP/1.1
OData-Version: 4.0
OData-MaxVersion: 4.0
Content-Type: application/json;odata.metadata=minimal
Accept: application/json;odata.metadata=minimal
Accept-Charset: UTF-8
User-Agent: Microsoft.OData.Client/7.9.2

{"@odata.type":"#Ex113.Models.TestA","Id":1}
--changeset_cd219c83-deef-4b21-a6cf-59f0b8d960e3
Content-Type: application/http
Content-Transfer-Encoding: binary
Content-ID: 2

POST http://localhost/odata/TestBs HTTP/1.1
OData-Version: 4.0
OData-MaxVersion: 4.0
Content-Type: application/json;odata.metadata=minimal
Accept: application/json;odata.metadata=minimal
Accept-Charset: UTF-8
User-Agent: Microsoft.OData.Client/7.9.2

{"@odata.type":"#Ex113.Models.TestB","Id":1,"Value":13}
--changeset_cd219c83-deef-4b21-a6cf-59f0b8d960e3
Content-Type: application/http
Content-Transfer-Encoding: binary
Content-ID: 3

POST $1/TestBs/$ref HTTP/1.1
OData-Version: 4.0
OData-MaxVersion: 4.0
Accept: application/json;odata.metadata=minimal
Accept-Charset: UTF-8
Content-Type: application/json;odata.metadata=minimal
User-Agent: Microsoft.OData.Client/7.9.2

{"@odata.id":"$2"}
--changeset_cd219c83-deef-4b21-a6cf-59f0b8d960e3--
--batch_c6718d3f-c42b-4650-bbf2-acf33e38c65d--

@mikepizzo

mikepizzo commented Feb 11, 2022

Copy link
Copy Markdown
Contributor

Okay; I see the disconnect. This does fix the absolute Uri issue, but there is a related issue that also needs to be addressed, which is that $ids can reference any request, not just in the same changeset (that was an old restriction that has been relaxed). So, for example, the repro in #360 would still not work, because the two requests are not grouped together in an atomicityGroup.

I have a fix for that, as well as the relative URL issue that this fixes, which I'll share in a draft PR and we can discuss...

@gathogojr

Copy link
Copy Markdown
Contributor Author

$ids can reference any request, not just in the same changeset (that was an old restriction that has been relaxed)
@mikepizzo I wasn't aware of that. Would you know if we ever spec'ed and implemented that? Can the customer apply the atomicityGroup such that the fix for the relative Url issue works for them in the interim?

@mikepizzo

Copy link
Copy Markdown
Contributor

See #482 for an alternate approach.

  1. Rather than create a relativeUri by prepending "/", Fix reference handling in $batch #482 uses the baseUri to create an absolute URI.
  2. Shares ContentIdToLocationMapping across the request, not just within a changeset.

@gathogojr

gathogojr commented Feb 11, 2022

Copy link
Copy Markdown
Contributor Author

See #482 for an alternate approach.

  1. Rather than create a relativeUri by prepending "/", Fix reference handling in $batch #482 uses the baseUri to create an absolute URI.
  2. Shares ContentIdToLocationMapping across the request, not just within a changeset.

@mikepizzo I see a potential problem here. Here is the scenario:
POST resource A => returns location (Url) for resource A - http://tempuri.org/As(1)
POST resource B => returns location (Url) for resource B - http://tempuri.org/Bs(1)
POST link A to B => Url needs to be http://tempuri.org/As(1)/Bs/$ref where Bs is a navigation property

If all the requests are in the same batch and you append base Uri at the point you're doing it, before we have resolved the location for resource A (that corresponds to $1 in the Url for the /$ref request), will that give us the desired Url for the /$ref request?

@gathogojr

Copy link
Copy Markdown
Contributor Author

#481 (comment)
Let's verify that with a test. I'll push one shortly then you can add it to your PR

@gathogojr

Copy link
Copy Markdown
Contributor Author

Addressed in #482

@gathogojr gathogojr closed this Feb 11, 2022
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.

Invalid Uri for $ref request in a batch request with single changeset

2 participants