-
Notifications
You must be signed in to change notification settings - Fork 9
Switch requests to path first URLs #122
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
base: develop
Are you sure you want to change the base?
Conversation
…efactor selectRows, makeFilter.
…e both newer and legacy filter formats. Update remaining functions to use the new labkey.buildURL utility.
cnathe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving. I'm planning to pull down the changes now to do manual testing. I'll focus mainly on the selectRows colFilters as most of the other changes for the path first part and labkey.buildURL seem like they should be covered by the existing tests.
| baseUrl=labkey.getBaseUrl(baseUrl) | ||
|
|
||
| ## check required parameters | ||
| if (missing(baseUrl) || is.null(baseUrl) || missing(folderPath)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are also checking for missing baseUrl and folderPath in labkey.buildURL. however, here we also check for is.null on baseUrl. should that check also be happening in labkey.buildURL for the various params?
| if(schemaName==URLdecode(schemaName)) {schemaName <- URLencode(schemaName)} | ||
| if(queryName==URLdecode(queryName)) {queryName <- URLencode(queryName)} | ||
| if(is.null(lookupKey)==FALSE) {if(lookupKey==URLdecode(lookupKey)) lookupKey <- URLencode(lookupKey)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't have to do this URLencode check anymore with the labkey.buildURL? excellent!
| { | ||
| url <- parse_url("") | ||
| url$query <- filters[i] | ||
| myurl <- build_url(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build_url is the part that handles the encoding part now right?
|
Manual testing Issue with special character: @labkey-klum For _> Error in makeDF(mydata, colSelect, showHidden, colNameOpt) :
I believe this is mainly from the column with name "c,c". For _> Error in parseToList(colFilter, dataRegionName = "", urlDecode = TRUE) :
|
Rationale
labkey.buildURLto handle URL generation. This includes moving the folderPath validation into this method and out of the calling function. Leverage thehttrlibrary to generate the URL including any query parameters and URL encoding.selectRowsto consolidate parameter generation for both GET and POST methodsmakeFilterto support generating filter lists using named elements. This is the format that thehttrutilities can accept for URL params more readily. While we aren't making use of this variant in this update, the plan is to eventually migrate to this format by default in a future release of Rlabkey. Until then, we support both the legacy and newer formats for functions that consume the return value ofmakeFilter.Related Pull Requests
LabKey/testAutomation#2857