A few bugfixes - Githubissues

a8m / documentdb

Go driver for Microsoft Azure DocumentDB

MIT License

33 stars 25 forks source link

A few bugfixes #19

Closed harikb closed 5 years ago

harikb commented 5 years ago

cc @pavelsmejkal since he might be working on other changes.

Each commit in the pull request is an individual bug fix.

Content-Length header was corrupted. Probably azure receiving side was tolerant to this optional header.
Cross-partition query header needed a typo fix to both key and value "True vs true" etc
Queries without parameters to documentdb.NewQuery() created a "parameters: null" in the serialized output (azure barfs on it)
Upsert (POST) to existing document returns 200. This may need further tuning if it optionally returns 200 or 201 . This has been tested with my code.

harikb commented 5 years ago

This is a rebased attempt of #18 - API had lot of changes. This new pull request only changes very few lines. One fix for PartitionKey support was unnecessary after prior pull request changes.

a8m commented 5 years ago

Looks good to me. Let's wait for @pavelsmejkal approval.

We definitely need to work on a real integration test with Azure API.

a8m commented 5 years ago

@pavelsmejkal - I can't add you as a reviewer until you will approve the Github invitation.

harikb commented 5 years ago

Turns out I have to allow for Upsert() to have either 200 or 201 on success. What do you think of making it a statuses []int wherever status is passed? (method(), do() etc) I am also ok with a map[int]bool ?

harikb commented 5 years ago

@pavelsmejkal @a8m would a change like this be acceptable? https://gist.github.com/harikb/aea4f86cee8e2cd540f130579d30ebaf

This is not in this pull request, but I can add it.

Both method and do are unexported method and so there is no API breakage.

a8m commented 5 years ago

Turns out I have to allow for Upsert() to have either 200 or 201 on success. What do you think of making it a statuses []int wherever status is passed? (method(), do() etc) I am also ok with a map[int]bool ?

I think that in general 2xx means that the request accepted proceeded. Do we really care if it's 200, 201 or 204?

pavelsmejkal commented 5 years ago

Sorry guys, will check asap

pavelsmejkal commented 5 years ago

It looks good and thank you for correction. I will more focus on quality then quantity.

pavelsmejkal commented 5 years ago

about your gist we can also address it this way: PR #20.

a8m commented 5 years ago

Thanks @harikb for your contribution. I've added you as a collaborator, you're welcome to accept this in your mail.