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

[CALCITE-1025] Add support for HTTP Basic auth (for proxies) in Avati… #180

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@mindcrime

mindcrime commented Dec 14, 2015

Implementation of the feature request from CALCITE-1025 JIRA. If a username/password are supplied, this code creates an HTTP BASIC header and adds the credentials to the HTTP request. This allows Calcite / Avatica based drivers (like the Phoenix "thin" driver) to pass through HTTP proxies like Knox.

@julianhyde

This comment has been minimized.

This comment has been minimized.

Owner

mindcrime replied Dec 15, 2015

Aah, yeah, protected would probably have been fine. That was just force of habit on my part, to default things to the least visibility.

This comment has been minimized.

julianhyde replied Dec 15, 2015

OK, fine, leave it, we'll sort it out later.

My usual instinct is to make things public and immutable; but we don't have Guava ImmutableList in avatica because we're trying to keep the dependencies minimal.

@julianhyde

This comment has been minimized.

Contributor

julianhyde commented Dec 14, 2015

I might be mistaken, but it looks as if the code does basic authentication even if user and password as not specified. Is this what we want?

Can a user choose not to use basic authentication?

Do we need some changes to http://calcite.apache.org/docs/avatica_overview.html ? At least "basic authentication" on the "Implemented" list.

This change needs some tests, including one where the password is valid, another where it is invalid, and another where user name and password are not specified.

@mindcrime

This comment has been minimized.

mindcrime commented Dec 15, 2015

Aaargh, that's my bad. The other version of the patch has the conditional logic in it. I did this one first and then forgot to bring that change back over. Should I just close this pull request and submit a new one with the correct version? Or leave this as is and do a separate pull request that wraps this in conditional logic?

@mindcrime

This comment has been minimized.

mindcrime commented Dec 15, 2015

I'll work up some tests as well. It might take me a little while to look at how the existing tests work and get something going (other than this small change, I haven't spent a lot of time looking at the Calcite code).

@julianhyde

This comment has been minimized.

Contributor

julianhyde commented Dec 15, 2015

That's why we insist on tests! It's easy to see what is supposed to work.

Let me know if you need help figuring out where & how to write the tests.

I believe that if you make additional commits to this branch they automatically become part of the pull request.

By the way, to modify the doc you should modify site/_docs/avatica_overview.md. Don't worry if you don't know markdown; just provide the content and I'll make it look good.

@mindcrime

This comment has been minimized.

mindcrime commented Dec 15, 2015

OK, sounds good.

@julianhyde

This comment has been minimized.

Contributor

julianhyde commented Jul 2, 2018

@mindcrime Can you please close this PR? https://issues.apache.org/jira/browse/CALCITE-1025 has been marked a duplicate of https://issues.apache.org/jira/browse/CALCITE-1173, which was fixed in avatica-1.8. And in any case, this code has since been moved to the Avatica project.

@mindcrime mindcrime closed this Jul 5, 2018

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