chore: store oauth2 introspection result as bytes in cache #811
Conversation
|
|
j3r0lin
changed the title
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.
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. |
|
|
||
| a.tokenToCache(config, i, "token", fosite.WildcardScopeStrategy) | ||
| // wait cache to save value | ||
| time.Sleep(time.Millisecond * 10) |
There was a problem hiding this comment.
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.
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.
🤦♂️ 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