Breeze / breeze.js.samples

Breeze JavaScript Client sample applications
MIT License
96 stars 85 forks source link

Relative URLs within odata $batch requests receive 404; SaveChanges fails #31

Open johncrim opened 9 years ago

johncrim commented 9 years ago

To reproduce:

  1. Start with Breeze/breeze.js.samples/net/ODataBreezejsSample
  2. nuget update Microsoft.AspNet.WebApi.OData to 5.3.1
  3. Run the app; navigate to /Home/Bowa; try deleting a Todo list, or adding a new item to a ToDo list. SaveChanges will fail.

The request:

--batch_3282-396a-353f
Content-Type: multipart/mixed; boundary=changeset_b5e1-6019-8b2f

--changeset_b5e1-6019-8b2f
Content-Type: application/http
Content-Transfer-Encoding: binary

POST odata/TodoItems HTTP/1.1
Content-ID: 1
DataServiceVersion: 2.0
Accept: application/atomsvc+xml;q=0.8, application/json;odata=fullmetadata;q=0.7, application/json;q=0.5, */*;q=0.1
Content-Type: application/json
MaxDataServiceVersion: 3.0

{"Id":-2,"TodoListId":4,"Description":"falalalala","IsDone":false}
--changeset_b5e1-6019-8b2f--

--batch_3282-396a-353f--

The response:

HTTP/1.1 202 Accepted
Cache-Control: no-cache
Pragma: no-cache
Content-Type: multipart/mixed; boundary=batchresponse_7e404dc1-7370-4f23-a33f-95e50da07c96
Expires: -1
Server: Microsoft-IIS/8.0
DataServiceVersion: 3.0
X-AspNet-Version: 4.0.30319
X-SourceFiles: =?UTF-8?B?Qzpcc3JjXGdpdGh1YlxCcmVlemVcT0RhdGFTYW1wbGUrRW50aXR5UmVwb3NpdG9yeVxPRGF0YUJyZWV6ZWpzU2FtcGxlXG9kYXRhXCRiYXRjaA==?=
X-Powered-By: ASP.NET
Date: Thu, 25 Dec 2014 21:33:14 GMT
Content-Length: 681

--batchresponse_7e404dc1-7370-4f23-a33f-95e50da07c96
Content-Type: multipart/mixed; boundary=changesetresponse_7915c1a7-3901-445f-b8bc-12cc49605dec

--changesetresponse_7915c1a7-3901-445f-b8bc-12cc49605dec
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 404 Not Found
Content-ID: 1
Content-Type: application/json; charset=utf-8

{"Message":"No HTTP resource was found that matches the request URI 'http://localhost:55802/odata/odata/TodoItems'.","MessageDetail":"No type was found that matches the controller named 'odata'."}
--changesetresponse_7915c1a7-3901-445f-b8bc-12cc49605dec--
--batchresponse_7e404dc1-7370-4f23-a33f-95e50da07c96--
johncrim commented 9 years ago

Note that updating the Breeze dependencies to latest doesn't fix the problem.

wardbell commented 9 years ago

Which OData adapter are you using? And which version of OData on the server? Web API v4 OData requires the derivative adapter specifically for that server, named webApiOdata4. I've updated the OData documentation to make this more clear. HTH.

johncrim commented 9 years ago

For this, I'm using the exact code from the BOWA sample, with an updated dependency - which I'm pretty sure is OData 3, so 'webApiOData' is in use. I'll also use the same adapter for my company's uses.

I also tried 'webApiOData4', but it looks like it is not the right implementation given the version of datajs being used; and it might also expect a different $metadata format.

From poking around in the source, it looks like what is needed (as a workaround) is an ability to replace webApiODataCtor.prototype.getRoutePrefix with an implementation that returns dataService.serviceName - I just haven't yet figured out how to do that without modifying the source.

I expect that the BatchHandler in the WebApi.OData project was updated to no longer support incorrect relative URLs... I'll see if I can find that changeset.

wardbell commented 9 years ago

In the docs I explain how to monkey patch getRoutePrefix w/o changing the source

johncrim commented 9 years ago

That's great - thank you very much for the assistance. That's good enough to allow me to continue working.

However, I think it's worth asking the questions:

  1. Is the implementation of getRoutePrefix correct (for both webApiOData and webApiOData4 - since both use the same impl). I agree that it's frustrating that something that worked before stopped working, but the question is whether it was correct before, or a bug that it worked before.
  2. Is there an implementation of getRoutePrefix that is straightforward, correct and works both for the old version of web API odata (version 5.2.2), and the new version of web API odata (version 5.3.1).

IMO the handling of relative URLs within the batch was incorrect before. eg it makes sense to me that relative URLs should be resolved relative to the containing $batch URL - so using standard relative URL resolution: http://host/odata/$batch + odata/TodoItems = http://host/odata/odata/TodoItems which is clearly not the intended URL. IMO resolving odata/TodoItems differently was a web api odata bug before, which is now fixed.

The odata 4.0 spec (http://docs.oasis-open.org/odata/odata/v4.0/errata02/os/complete/part1-protocol/odata-v4.0-errata02-os-part1-protocol-complete.html#_Toc406398361) specifies that relative paths should be resolved relative to the batch request URI.

I think the best way to avoid this sort of issue is to not use this type of relative URL - either use absolute URLs, or use absolute paths (with the / prefix). The odata 3.0 spec on batch handling (http://www.odata.org/documentation/odata-version-3-0/batch-processing/) doesn't specify how request URIs within a batch should be resolved. The example in the spec uses absolute paths, with the last one missing the leading / - which is an unfortunate omission. IMO that is a spec bug, and shouldn't be interpreted as /service/$batch + GET service/Products = /service/Products.

Long story short - if you change the implementation to not strip the / prefix:

  webApiODataCtor.prototype.getRoutePrefix = function (dataService) {
    // Get the routePrefix from a Web API OData service name.
    // The routePrefix is presumed to be the pathname within the dataService.serviceName
    // Examples of servicename -> routePrefix:
    //   'http://localhost:55802/odata/' -> '/odata/'
    //   'http://198.154.121.75/service/odata/' -> '/service/odata/'
    if (typeof document === 'object') { // browser
      var parser = document.createElement('a');
      parser.href = dataService.serviceName;
    } else { // node
      parser = url.parse(dataService.serviceName);
    }
    var prefix = parser.pathname;
    if (prefix.substr(-1) !== '/') {
      prefix += '/';
    }      // ensure trailing '/'
    return prefix;
  };

it works correctly both with the older version of web api odata, and with the newer version (and presumably also works for web api odata 4.0 - since it matches the spec. I haven't tested that.)

Alternatively full URIs work (with the older and newer versions of web api odata):

  webApiODataCtor.prototype.getRoutePrefix = function (dataService) {
    return dataService.serviceName;
  };

But I think the former version with absolute paths is preferable.

wardbell commented 9 years ago

I will continue to noodle this. It is madness to have multiple adapters that differ only in this one respect.

I went through many of the mental gyrations you describe and almost settled on absolute paths .. which AFAIK is not endorsed by any spec but neither is it forbidden and seems to work everywhere. But I backed off, afraid I'd unknowingly break someone.

There are many more significant differences to absorb having to do with the variety of OData specs supported. My sense is that we'll soon be rethinking the OData adapters to deal with OData versioning. We'll take up this comparatively minor issue at that time.

Thanks for helping and you're continuing involvement with Breeze.

DO stay after us about progress on the OData front. We lose heart when the community seems indifferent.

ghost commented 9 years ago

Just a note on my findings with the same issue. I was having the same problem. The fix reported by johncrim worked great for Chrome, but not IE. To fix for IE and Chrome, I just added:

return "";

To the beginning of the webApiODataCtor.prototype.getRoutePrefix function.

wardbell commented 9 years ago

Hey folks. I'm sympathetic and all. But there is no need to patch Breeze to workaround the present problem as was suggested in this StackOverflow post.

To save you the trouble, I'm reprinting my response here. I hope you can bear with us as we try to get things straightened out with the Microsoft OData team:


YOU SHOULD NOT PATCH BREEZE!!!

It is not necessary to solve this problem and patching breeze complicates your ability to upgrade as we release new versions which we do about every 2 or 3 weeks.

We are aware of the issue you raise and the discussion at https://github.com/Breeze/breeze.js.samples/issues/31. We're going to do something about it ... we just haven't decided exactly what we'll do.

But meanwhile, you can apply your preferred workaround without changing Breeze.

That's because we made getRoutePrefix a published extension point of the "webApiOData" adapter.

For example, you can do almost exactly as @user2908937 suggests without changing a line of Breeze source.

Simply overwrite that extension method in your application setup logic ... like this.

function configureBreeze() {

    // choose and get the Web API OData DataServiceAdapter
    var adapter = breeze.config.initializeAdapterInstance(
                  'dataService', 'webApiOData', true);

    // override the getRoutePrefix instance method
    adapter.getRoutePrefix = function (dataService) {
        return "";
    };

    // ... more configuration ...
}

This isn't hacking. This is exactly why we exposed this method.

Why don't we just fix it?

Good question.

The ASP.NET Web API OData implementation has undergone so much churn and so many breaking changes that we've been holding back on trying to keep up with until we see stability and proper behavior. The inside-the-$batch url is one of the things they broke recently. It is causing us pain but it is a tiny issue compared to everything else they've broken. We'd prefer a comprehensive solution to death by 1000 cuts.

Recent conversations with the MS team have been encouraging. We intend to try to sync up again soon.

Meanwhile, you're well positioned to take care of this particular issue in the manner I suggested here.

rapmue commented 9 years ago

@wardbell thanks for the explanation. maybe the example at http://www.getbreezenow.com/samples/breeze-web-api-odata should be expanded too?

wardbell commented 9 years ago

Will look at that when we upgrade our OData support. Thanks.

hemantsathe commented 9 years ago

I get the 404 errors on internal batch requests and I am not able to figure out why. I have implemented the fixes as well. No luck whatsoever. The API route in the batch works fine as single request. I am using OData v3. On top of this, the Enum to string conversions are also failing for me. Very frustrated. Could it be that I have model. DbContext and API all in separate assemblies?

wardbell commented 9 years ago

I THINK I JUST FIGURED THIS OUT!

I'm talking only about OData v.3; v.4 is still a work-in-progress.

Apparently the MS OData team made a breaking change somewhere around the Microsoft.AspNet.WebApi.OData v.5.3.1 package. I didn't have a chance too look into it until yesterday.

I'm not saying that it was a bad change. I agree that the previous behavior was wrong and I told the team that long ago. They disagreed. That's why I invented getRoutePrefix. Then they silently seem to have come around.

If I'm correct, the problem occurs when you add a new record inside a $batch request. The 404 concerns the request embedded in the $batch payload and reads something like this:

"No HTTP resource was found that matches the request URI 'http://localhost:55802/odata/odata/TodoItems' ..."

Notice the double "odata/odata/". It used to be just "odata/". Grrrrr.

You can eliminate this particular problem by replacing webApiODataCtor.prototype.getRoutePrefix with a function that returns an empty string but then modify and delete may fail.

I took @johncrim 's advice and switched to generating absolute URLs everywhere, both in direct calls and inside $batch payloads.

As he notes, the spec is mum on whether absolute URLs can be used everywhere. In practice it has been fine for all OData source I've tried so far.

I threw away the getRoutePrefix method.

The adapter now uses a new webApiODataCtor.prototype.getAbsoluteUrl method everywhere. This appears to work for all known Web API OData versions.

We still have to test it for WCF OData sources. I'm hopeful it will work also for them as is. If not, we can make a small adjustment to get over that hill.

I committed the change as e7cb67e.

You can try it with a contingent version of breeze.debug.js.

I expect (baring a nasty surprise) that this will be part of our official v.1.5.5 release.