fixed _bulk_get headers #4567

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
@wilk
Contributor

wilk commented Nov 12, 2015

If host.headers is undefined, don't add it to the ajax call because it will perform the _bulk_get call without basic headers (like the authorization one).

When this problem happens, _bulk_get is done without authorization header and a CORS is thrown even if credentials are passed from the application and CORS are enabled on the server.

fixed _bulk_get headers
If host.headers is undefined, don't add it to the ajax call because it will perform the _bulk_get call without basic headers (like the authorization one).
@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 12, 2015

Member

We use this pattern in all of our calls, so I would like to understand the issue a bit better. Are you saying you do a

new PouchDB('name', {ajax: {headers: { 'Auth': ',,,'}});

then when making the bulk_docs request the headers are not sent?

Member

daleharvey commented Nov 12, 2015

We use this pattern in all of our calls, so I would like to understand the issue a bit better. Are you saying you do a

new PouchDB('name', {ajax: {headers: { 'Auth': ',,,'}});

then when making the bulk_docs request the headers are not sent?

@wilk

This comment has been minimized.

Show comment
Hide comment
@wilk

wilk Nov 12, 2015

Contributor

I tried with both:

new PouchDB('name', {ajax: {headers: {Authorization: ...}}})
db.replicate.from(url, {ajax: {headers: {Authorization: ...}}})

The problem occurs only on iOS (9) because in Chrome browser it works fine.
In fact, headers are well sent with allDocs call but not with bulk get.

Contributor

wilk commented Nov 12, 2015

I tried with both:

new PouchDB('name', {ajax: {headers: {Authorization: ...}}})
db.replicate.from(url, {ajax: {headers: {Authorization: ...}}})

The problem occurs only on iOS (9) because in Chrome browser it works fine.
In fact, headers are well sent with allDocs call but not with bulk get.

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 12, 2015

Member

ok so this is actually a curious bug, _bulk_get landed around the same time I refactored the way some of the header merging works, previously every call to ajax( from inside http used to have to pass in global headers but now they are merged within the ajax function (https://github.com/pouchdb/pouchdb/blob/master/lib/adapters/http/index.js#L156).

So if you just remove host.headers here completely you should be good, @wilk do you fancy testing that and editing the PR if that works?

Member

daleharvey commented Nov 12, 2015

ok so this is actually a curious bug, _bulk_get landed around the same time I refactored the way some of the header merging works, previously every call to ajax( from inside http used to have to pass in global headers but now they are merged within the ajax function (https://github.com/pouchdb/pouchdb/blob/master/lib/adapters/http/index.js#L156).

So if you just remove host.headers here completely you should be good, @wilk do you fancy testing that and editing the PR if that works?

@wilk

This comment has been minimized.

Show comment
Hide comment
@wilk

wilk Nov 12, 2015

Contributor

You mean, removing it from doBulkGet?
Is it correct?
I mean, I've already tested it without host.headers and it worked but I refactored just for backwards compatibility.
Are host.headers useless?

Contributor

wilk commented Nov 12, 2015

You mean, removing it from doBulkGet?
Is it correct?
I mean, I've already tested it without host.headers and it worked but I refactored just for backwards compatibility.
Are host.headers useless?

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 12, 2015

Member

Yup pretty much, we moved where we defined the host.headers that are used, every call to ajax used to have them, now none of them do and the ajax function handles that for them all

Member

daleharvey commented Nov 12, 2015

Yup pretty much, we moved where we defined the host.headers that are used, every call to ajax used to have them, now none of them do and the ajax function handles that for them all

removed host.headers
host.headers are no useful anymore
@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 12, 2015

Member

@wilk Hey sorry to be hassle, however the refactoring here is different to how most of our code is structured, we dont use comma seperated var's and we generally stick to calling ajax with its options inline, partially to make up for the confusing api to that that we should fix.

If you could edit it to just take out the host.headers line from the original, that would be perfect, thanks

Member

daleharvey commented Nov 12, 2015

@wilk Hey sorry to be hassle, however the refactoring here is different to how most of our code is structured, we dont use comma seperated var's and we generally stick to calling ajax with its options inline, partially to make up for the confusing api to that that we should fix.

If you could edit it to just take out the host.headers line from the original, that would be perfect, thanks

adjusted code style
adjusted code style to the rest of the project
@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 13, 2015

Member

Awesome, thanks for that, apologies it was a little roundabout, merged in 3011e71

Member

daleharvey commented Nov 13, 2015

Awesome, thanks for that, apologies it was a little roundabout, merged in 3011e71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment