-
-
Notifications
You must be signed in to change notification settings - Fork 238
add ValidateSession() to Session interface #1288
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
Conversation
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.
Code looks great. I think this is a nice hook for GMS integrators to have.
The only suggestion I have is that it would be really cool if we could test that the new ValidateSession method is being called. We've got tests in the Dolt layer of course, but it would be awesome to have a quick tests in this layer that asserts that, too. The TestPersist test method has an example of using a SessionFactory function to create in memory sessions; that might be close to what we need. Maybe the InMemory session type could be configured with a callback function to call from the ValidateSession method that would increment a counter or something we could use just as a sanity check that the callback is being invoked correctly?
Co-authored-by: Jason Fulghum <jason@dolthub.com>
enginetest/memory_session.go
Outdated
| type InMemoryBaseSession struct { | ||
| Session sql.Session | ||
| idx int | ||
| } |
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.
Is it possible to use InMemoryPersistedSession instead of creating/maintaining this new type?
Could we create a new constructor method, like NewInMemoryPersistedSessionWithValidationCallback that takes a function that ValidateSession could just call?
Other than that, I thought this test looked great!
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.
Looks good! Thanks for making sure we can test this functionality at the GMS layer, too.
dolthub/dolt#4395 depends on this PR