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

fix_382 #383

Merged
merged 10 commits into from Jun 14, 2023
Merged

Conversation

ethanbsmith
Copy link
Contributor

@ethanbsmith ethanbsmith commented Apr 19, 2023

adds a lightweight session for use on yahoo quote requests
session is needed for both the cookies and the "crumb" field
the handle from the session must also be used for subsequent requests
fixes #382

@pstadler
Copy link

pstadler commented Apr 21, 2023

I thought I'd be a nice neighbor and let you know that certain regions (probably EU) seem to get redirected to a consent page during the initial call, thus failing to obtain a session. We're currently investigating possible solutions:
pstadler/ticker.sh#33 (comment)

@ethanbsmith
Copy link
Contributor Author

thx @pstadler. it looks like the cookies and crumbs are no longer needed again for v7 (at least for now). r u seeing the redirects on calls to the v7 api w/out simulating a session?

@pstadler
Copy link

it‘s happening over at my project using something very similar to your fix. Going to remove it again if it‘s working again without crumb. Wondering for how long though, it‘s been flaky for a while.

@pstadler
Copy link

I gladly reverted the change on my end. Fingers crossed that it won't happen again.

@ethanbsmith
Copy link
Contributor Author

im currently testing an update that caches the yahoo handle. i dont know what the error will ben when the handle becomes invalid, as this is probably driven by a timeout on the yahoo server side, so i cant force the error condition. will push once i have that part worked out

@pstadler
Copy link

pstadler commented May 6, 2023

my assumption is that you‘ll get a 401 Unauthorized if the session expires (if this is what you’re referring to). In this case I‘d simply try to obtain a new session and fetch the quotes again. See https://github.com/pstadler/ticker.sh/blob/54b73a3f57432b5efd102754c48f20ed246f8ed2/ticker.sh#L53

added handle caching across calls
@ethanbsmith

This comment was marked as outdated.

if ((r$status_code == 200) && (length(r$content) > 0)) {
ses$crumb = rawToChar(r$content)
} else {
if (!force.new) ses <- .yahooSession(TRUE) else stop("Unable to get yahoo crumb")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems backward. I expect to call .yahooSession(TRUE) when force.new is TRUE, else throw an error because we couldn't get a crumb.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its really a state machine. the force.new is never intended to be used directly, only in the recursive call. i can rewrite it as a loop to clean up the signature

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling the function recursively is fine. The logic on this line (51) confused me, but seems correct once I walk through it.

Assume force.new is FALSE. Line 45 checks that we have an active session. Line 51 checks if we're able to get a crumb. If we can't get a crumb, then !force.new is TRUE and we call .yahooSession(TRUE). So that seems like it does what it should.

I think some comments would help me remember what this is doing.

# we were unable to get a crumb
if (force.new) {
    # we couldn't get a crumb with a new session
    stop("unable to get yahoo crumb")
} else {
    # we tried to re-use a session but couldn't get a crumb
    # try to get a crumb using a new session
    ses <- .yahooSession(TRUE)
}
    

@joshuaulrich joshuaulrich merged commit 8be0ffa into joshuaulrich:master Jun 14, 2023
@joshuaulrich joshuaulrich added this to the Release 0.4.23 milestone Jun 14, 2023
joshuaulrich added a commit that referenced this pull request Jun 15, 2023
The logic is correct but confused me until I walked through it
carefully. The comments should keep future me from being confused.

Rename 'force.new' to 'is.retry' to make it clearer that the
call is a 'retry' call. Clean up existing comments.

See #382. See #383.
@ethanbsmith ethanbsmith deleted the 382_add_session_getQuote branch March 17, 2024 18:55
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

Successfully merging this pull request may close these issues.

getQuote.yahoo 401 error
3 participants