-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
chore: store oauth2 introspection result as bytes in cache #811
Conversation
Codecov Report
@@ Coverage Diff @@
## master #811 +/- ##
==========================================
+ Coverage 62.17% 62.37% +0.20%
==========================================
Files 102 102
Lines 4782 4787 +5
==========================================
+ Hits 2973 2986 +13
+ Misses 1534 1528 -6
+ Partials 275 273 -2
Continue to review full report at Codecov.
|
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.
Thank you for the contribution! I think this is going into the right direction.
It would be awesome if you could add tests! 😌
Tests are important to open source projects because the allow reviewers to better judge if the changes are doing what was intended. They also ensure that your changes never regress / break when someone else works on code that is related to this.
The value to be stored is created from the introspection call result, which may always could be marshaled to bytes, I have no idea how to generate a scenario to mock the marshal error branch. Do you have any suggestion about how to write this test case? |
Sorry for the late reply - the best way would probably be to create a test which modifies the struct after it was cached and then the cached value is incorrect! |
@aeneasr I think it's ready for review now. |
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.
Great job, thank you for your contribution!
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.
Sorry, I noticed one more thing!
|
||
a.tokenToCache(config, i, "token", fosite.WildcardScopeStrategy) | ||
// wait cache to save value | ||
time.Sleep(time.Millisecond * 10) |
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.
If you update ristretto to the newest version it has a new method called wait which ensures that the cache worker has completed it’s work. It is more reliable and less flaky than waiting here! Could you maybe use that instead? :)
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.
update ristretto to v0.1.0 will cause an issue: flag redefined: v.
which defined in :
# github.com/google/martian/init.go
var (
level = flag.Int("v", 0, "log level")
)
# github.com/google/golang/glog.go
func init() {
flag.BoolVar(&logging.toStderr, "logtostderr", false, "log to standard error instead of files")
flag.BoolVar(&logging.alsoToStderr, "alsologtostderr", false, "log to standard error as well as files")
flag.Var(&logging.verbosity, "v", "log level for V logs")
flag.Var(&logging.stderrThreshold, "stderrthreshold", "logs at or above this threshold go to stderr")
flag.Var(&logging.vmodule, "vmodule", "comma-separated list of pattern=N settings for file-filtered logging")
flag.Var(&logging.traceLocation, "log_backtrace_at", "when logging hits line file:N, emit a stack trace")
// Default stderrThreshold is ERROR.
logging.stderrThreshold = errorLog
logging.setVState(0, nil, false)
go logging.flushDaemon()
}
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.
🤦♂️ ok then keeping it as is is fine! Thank you :)
OAuth2 introspection result are using raw value to be stored in cache, but the cached value may be edit after stored into cache, which may cause go map concurrent write issue, so I changed the cached value to bytes using json serializer.
Related issue(s)
Checklist
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments