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

Add bpipe authentication with an application name #288

Merged
merged 17 commits into from Mar 26, 2019

Conversation

Projects
None yet
3 participants
@alfredkanzler
Copy link
Contributor

alfredkanzler commented Jan 29, 2019

Bloomberg's bpipe api allows authentication with an application name as described in "Bloomberg API Version 3.x Developers Guide" (https://data.bloomberglp.com/labs/sites/2/2014/07/blpapi-developers-guide-2.54.pdf) section 6.4.

Added functions to support this form of authentication. Please send questions to akanzler@drwholdings.com.

alfredkanzler added some commits Jan 28, 2019

This have been added or modified to allow the user to authenticate a …
…bpipe connection with

an application name. Description of authenticating a bpipe connection with an app is
described in the "Bloomberg API Version 3.x Developers Guide" in section 6.4
@johnlaing

This comment has been minimized.

Copy link
Contributor

johnlaing commented Jan 30, 2019

Is there a reason to create separate functions for this case? It seems simpler/preferable to incorporate an optional app name into the existing functions. But perhaps I'm missing something - I'm not familiar with the details of Bpipe.

@alfredkanzler

This comment has been minimized.

Copy link
Contributor Author

alfredkanzler commented Jan 30, 2019

There were a few reasons that I wrote new functions. Let me first say that I would rate myself as an intermediate level R developer at best. I have not developed full time in over ten years. I developed this because I needed to access bpipe. Now to the reasons

1.) the authenticate C++ function was structurally different enough that I thought it warranted a new function.
2.) I wanted to make my functions as C compatible as possible so did not want to add optional parameters or use function overloading. This precluded naming my blpConnect function blpConnect or adding an optional parameter. This was not a requirement but was my motivation.
3.) The only way I know to overload an R function is via the S3 class mechanism. The authenticate function had required parameters that I did not require to authenticate with an application. I would have had to recognize the desire to authenticate bpipe with an application name and call the correct C/C++ function. I did not know how to recognize that the function should be used to authenticate bpipe.

Those are the structural reasons. I also wanted to isolate my code since I do not consider myself a good package developer. Some of my assumptions above may not have been correct. If these are incorrect please inform me where my knowledge is lacking.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Mar 15, 2019

@alfredkanzler Very cool to this moving forward. I'll take a closer look at the code in the next day or two.

@johnlaing Can you give it a spin to run it at your end?

eddelbuettel added some commits Feb 21, 2019

release 0.3.9 (closes #290) (#291)
* release 0.3.9

* more URL updates needed bloomberglags no longer documents API
@eddelbuettel
Copy link
Member

eddelbuettel left a comment

PR looks great -- could not help myself and edited some whitespace, and also added Al to copyright header for those two files

Tried bringing it in line master by cherry-picking our commit, but it still thinks '8 ahead, 1 behind' so squash merge may be easiest.

Builds and checks cleanly on r-release and r-devel so no CRAN issues expected.
Remaining issue: @johnlaing to review and, if possible, run

con <- blpConnect_Impl(host, port, NULL)
} else {
con <- blpConnect_Impl(host, port, app_name)
}

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Mar 16, 2019

Member

Here the if (is.null(...)) and else branches could be collapsed into one which might be simpler. Yet this is clear.

Or it might be better to change it to two lines including one comment that null/non-null is handled at the C(++) level?

This comment has been minimized.

Copy link
@alfredkanzler

alfredkanzler Mar 16, 2019

Author Contributor

I think this is a good point. I can make the change on Monday. I like the simplification with a note in the documentation.

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Mar 16, 2019

Member

Sounds good! The code is in great shape but I want to hear from @johnlaing too.

if(uuid_ == R_NilValue) {
identity_p = authenticateWithApp(con_);
}
else {

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Mar 16, 2019

Member

I think we put our else on the same line but hey...

This comment has been minimized.

Copy link
@alfredkanzler

alfredkanzler Mar 16, 2019

Author Contributor

More than happy to follow your preferred format. Since I am going to make the change above I will make the change here.

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Mar 16, 2019

Member

See the 'Outdated' tag :) While I was at it I pushed some trivial changes back to your branch. So make sure you pull on Monday.

}
else {
identity_p = authenticateWithId(con_, uuid_, ip_address_);
}

This comment has been minimized.

Copy link
@eddelbuettel

eddelbuettel Mar 16, 2019

Member

I like the simpler switch here and the hand-off to these two specialised functions with the old default and new add-on. Nicely done.

@johnlaing

This comment has been minimized.

Copy link
Contributor

johnlaing commented Mar 18, 2019

This broke my connectivity right off the bat. I'm going to have to dig in here and figure out what's happening. Bear with me...

@alfredkanzler

This comment has been minimized.

Copy link
Contributor Author

alfredkanzler commented Mar 18, 2019

Can you send me how you connect? I will also dig into why it failed.

@johnlaing

This comment has been minimized.

Copy link
Contributor

johnlaing commented Mar 18, 2019

This was just a SAPI connection, so specifying host/port.

##' via \code{blpAuthenticate} after \code{blpConnect}.
##' @examples
##' \dontrun{
##' con <- blpConnect() # adjust as needed
##' }
blpConnect <- function(host=getOption("blpHost", "localhost"),
port=getOption("blpPort", 8194L),
default=TRUE) {
default=TRUE,
app_name = getOption("blpAppName", NULL)) {

This comment has been minimized.

Copy link
@johnlaing

johnlaing Mar 19, 2019

Contributor

Stylistic nitpick: we should use something other than snake_case for argument names. We've tended toward camelCase in most functions, thought there are also a few old-school R dot.separated names left over. I think appName fits best here.

// [[Rcpp::export]]
SEXP blpConnect_Impl(const std::string host, const int port, SEXP app_name_) {
Session* sp = NULL;
if (app_name_ == NULL) {

This comment has been minimized.

Copy link
@johnlaing

johnlaing Mar 19, 2019

Contributor

I think this should be R_NilValue rather than NULL. That seems to be what was killing my connection yesterday.

@alfredkanzler

This comment has been minimized.

Copy link
Contributor Author

alfredkanzler commented Mar 19, 2019

I missed fixing this. I will make the change and change to camel case. I should have this done today.

@alfredkanzler

This comment has been minimized.

Copy link
Contributor Author

alfredkanzler commented Mar 19, 2019

I changed to camel case in the R code. I also think the bug is fixed. John should test.

@johnlaing

This comment has been minimized.

Copy link
Contributor

johnlaing commented Mar 20, 2019

Bug is fixed! This is great, thanks for your work. Just a couple other cleanup items from me:

  • I don't think we need separate functions for blpConnect with/without app. The NoApp version is a subset of the WithApp, I'd be in favor of combining. (This is in contrast to authorization, where the internals are completely different and separate functions make perfect sense.)
  • In the R blpAuthenticate function we should pull the IP conversion inside the case statement, as it's irrelevant for the BPIPE code path.

I can take care of these easily on my end. And if we're all in agreement I can push back to your branch, Al, for one more quick test before merging.

@alfredkanzler

This comment has been minimized.

Copy link
Contributor Author

alfredkanzler commented Mar 21, 2019

This is good news. I unfortunately cannot test today. I will try to get setup to test tomorrow.

johnlaing and others added some commits Mar 21, 2019

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Mar 22, 2019

I made some teeny edits to ChangeLog (and DESCRIPTION) and resolved the merge conflict. Let's do the one final test and then it should be good to go to the repo ... and CRAN!

@alfredkanzler

This comment has been minimized.

Copy link
Contributor

alfredkanzler commented on f932bd1 Mar 22, 2019

I am slightly delayed but I hope to be setup on Monday or Tuesday to test.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Mar 22, 2019

Works for me. No need to rush this now that we did it so well.

@alfredkanzler

This comment has been minimized.

Copy link
Contributor Author

alfredkanzler commented Mar 26, 2019

I just completed testing without any issues in my tests.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Mar 26, 2019

I just merged in a dry-run in another repo and the graph is pretty impressive so I may change my plan and not not squash-merge. Any objections?

image

@alfredkanzler

This comment has been minimized.

Copy link
Contributor Author

alfredkanzler commented Mar 26, 2019

No objections from me

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Mar 26, 2019

@johnlaing All green from your end too?

@johnlaing

This comment has been minimized.

Copy link
Contributor

johnlaing commented Mar 26, 2019

All good here, in the sense that the code works. My preference is to always maintain commit history unless there's a very good reason to squash.

@eddelbuettel

This comment has been minimized.

Copy link
Member

eddelbuettel commented Mar 26, 2019

All good here, in the sense that the code works.

Ace. Will merge and release to CRAN (this eve or tomorrow eve). I'll probably call it 0.4.0 for good measure.

My preference is to always maintain commit history unless there's a very good reason to squash.

Yes, understood. It is a trade-off. In a squash the log of the individual commits become part of the one commit's log. So not erased, just a wee bit less visible. The upside is the saner graph. With a lot of concurrent activity those become harder to use.

@eddelbuettel eddelbuettel merged commit 9b75403 into Rblp:master Mar 26, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.