Skip to content
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

Merge #285 for default authentication support #286

Closed
randomee opened this issue Jan 5, 2019 · 9 comments
Closed

Merge #285 for default authentication support #286

randomee opened this issue Jan 5, 2019 · 9 comments

Comments

@randomee
Copy link
Contributor

randomee commented Jan 5, 2019

Mostly a discussion point (per Dirk) for #285

Explanation of what's changed... It's very, very, very similar to the existing default con code.

Change default args to blpAuthenticate() to support use getOptions() for args.

Change blpAuthenticate() to store the resulting identity to .pkgenv$blpAuth

Added one file/function - defaultAuthentication() which is more or less a copy/paste of defaultConnection(). But no stop() if no identity object is found. Since you don't need to have an identity for all connections. NULL is fine for desktop, and even some SAPI servers

Changed the defaults for some "data" functions (e.g. bds() bdp() subscribe() to use identity = defaultAuthentication() instead of identity = NULL

Added defaultAuthentication() to NAMESPACE.

Added documentation to vignette and to the function files.

The only outstanding bit is some of the data functions don't have support for identity (see #284 ). Once they do, they only need to use identity = defaultAuthentication() akin to the other data functions.

It's arguable where blpAutoAuthenticate should fit/nest in init.R I think blpAutoAuthenticate should be dependent on blpAuthConnect. It doesn't make any sense to authenticate w/o a connection. It will always fail. I think setting blpAutoAuthenticate should auto-set blpAuthConnect. But I'm good however.

Tested from Linux (Ubuntu 18.04LTS) against a desktop (DAPI) host, and a SAPI host. I don't have access to B-PIPE, but it should work since nothing changed on the C++/lib end.

Example of the code working

R version 3.4.4 (2018-03-15) -- "Someone to Lean On"
Copyright (C) 2018 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> testIP <- "x.x.x.x"
> testUUID <- "xxxxxxxx"
> testSAPI <- "x.x.x.x"
> options("uuid" = testUUID)
> options("blpIP" = testIP)
> options("blpHost" = testSAPI)
> options("blpAutoConnect" = TRUE)
> options("blpAutoAuthenticate" = TRUE)
>
> library(Rblpapi, quietly=TRUE)
Rblpapi version 0.3.8.1 using Blpapi headers 3.8.18.1 and run-time 3.8.18.1.
Please respect the Bloomberg licensing agreement and terms of service.
> bdp("IBM US Equity", "NAME")
                                     NAME
IBM US Equity INTL BUSINESS MACHINES CORP
>

Connecting w/o auto connection/auth

R version 3.4.4 (2018-03-15) -- "Someone to Lean On"
Copyright (C) 2018 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> testIP <- "x.x.x.x"
> testUUID <- "xxxxxxxx"
> testSAPI <- "x.x.x.x"
>
> library(Rblpapi, quietly=TRUE)
Rblpapi version 0.3.8.1 using Blpapi headers 3.8.18.1 and run-time 3.8.18.1.
Please respect the Bloomberg licensing agreement and terms of service.
> blpConnect(host=testSAPI)
> blpid<-blpAuthenticate(ip=testIP, uuid=testUUID)
> bdp("IBM US Equity", "NAME", identity=blpid)
                                     NAME
IBM US Equity INTL BUSINESS MACHINES CORP
>

Connecting to a desktop (no authentication needed) server (old codepath)

> testDesktop<-"x.x.x.x"
> options("blpHost" = testDesktop)
> options("blpAutoConnect" = TRUE)
> library(Rblpapi, quietly=TRUE)
Rblpapi version 0.3.8.1 using Blpapi headers 3.8.18.1 and run-time 3.8.18.1.
Please respect the Bloomberg licensing agreement and terms of service.
> blpConnect()
> bdp("IBM US Equity", "NAME")
                                     NAME
IBM US Equity INTL BUSINESS MACHINES CORP
@eddelbuettel
Copy link
Member

I think you are still missing the part where we suggest to file an issue ticket to establish what should be in a pull request before submitting one.

@johnlaing
Copy link
Contributor

As @randomee notes in the PR, this change is motivated by a comment I made in #251 long ago. It's a lot to process all at once, but I am following most of it and think it generally works.

On the issue of appropriate behavior given conflicting options: I don't think we should override the meaning of blpAutoConnect by forcing a connection just to authenticate. Instead I think the package should warn that authorization could not be done because there was no connection.

@randomee
Copy link
Contributor Author

randomee commented Jan 6, 2019

Thanks for having a look John. I'm more than happy to explain anything that doesn't immediately make sense. And I'm happy to test/edit as needed as well.

The current code does what you would like wrt auto auth w/o auto connect. It could be made more clean if that's desired.

> options("uuid" = testUUID)
> options("blpIP" = testIP)
> options("blpHost" = testSAPI)
> options("blpAutoAuthenticate" = TRUE)
> library(Rblpapi, quietly=TRUE)
Rblpapi version 0.3.8.1 using Blpapi headers 3.8.18.1 and run-time 3.8.18.1.
Please respect the Bloomberg licensing agreement and terms of service.
Error: package or namespace load failed for ‘Rblpapi’:
 .onAttach failed in attachNamespace() for 'Rblpapi', details:
  call: NULL
  error: No connection object has been created. Use 'blpConnect()' first.
>

@randomee
Copy link
Contributor Author

Updated to merge to current head (conflicts arose from the bpipe commits).

reverted the ip.address change that John pointed to being problematic.

tested new code from linux against

  • DAPI (port forwarded from windows) with explicit args
  • DAPI using options() defaults and blpAutoConnect
  • SAPI with explicit args
  • SAPI using options() defaults and blpAutoConnect/blpAutoAuthenticate
  • SAPI using explicit application ID
  • SAPI using options() defaults

Alas, I don't have a bpipe to test against.

Happy to discuss, or make changes as requested.

@eddelbuettel
Copy link
Member

@alfredkanzler Could you possibly check if there are any side effects with bpipe?

@alfredkanzler
Copy link
Contributor

alfredkanzler commented May 20, 2019 via email

@eddelbuettel
Copy link
Member

Well I can't currently run it so I rely on all of you in the crew to tell me if something where to be fishy. This looked clean from glancing at the diff though...

@alfredkanzler
Copy link
Contributor

I recently had a conversation with a user in Canada. He was having problems because he did not set his identity. I sent him a code snippet and he was able to successfully run bpipe. He did not seem to have any side effects. I also have not noticed any side effects.

I am also running this is a shiny application and it seems to run without issues.

Is there and issue with the original path. I will try running on Windows tomorrow with the original path.

@alfredkanzler
Copy link
Contributor

I just ran the bdh on the original path and through bpipe. Both ran successfully.

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

No branches or pull requests

4 participants