fix: whoami regression to anonymous user#4303
Conversation
|
|
||
| #[test] | ||
| #[cfg(all(unix, not(target_arch = "wasm32")))] | ||
| fn it_uses_the_os_username_when_url_omits_user() { |
There was a problem hiding this comment.
I didn't copy the Mutex bit because I didn't understand why it was needed. This test was enough to fail without the fix, so I think it could work as the regression test needed and avoid a separate CI step.
What do you think? If the mutex was really needed, do you mind explaining why?
|
Seems reasonable. The purpose of the guard was to unset PGUSER while
ensuring it's restored to the previous value (if any) after the test has
run. (However, even this isn't perfect as @abonander points out in #4275,
because Rust tests can run in parallel.) The PGUSER variable is also read
when parsing the connection string, potentially causing the test to pass
even when the system username can't be read.
I see two approaches here, and we may need input from a project maintainer
to decide:
1. Assume other tests don't set PGUSER, or if they do they make sure they
don't affect other tests as a side-effect (e.g., by disabling parallel
execution). With this approach, we don't need to worry about PGUSER and
your PR is good to go. Disadvantage: if any other tests set PGUSER now or
in the future, this new test may incorrectly pass, and the next regression
may go unnoticed again.
2. Clear PGUSER as in #4275, but then we'd ideally also ensure that doesn't
interfere with other tests, probably by running this one in its own process
(`cargo test` command) as @abonander suggested. Disadvantage: complexity.
Thanks for taking this over while I'm away!
|
|
@svnrb Got it, thanks for clarifying. One question, how about we read the PG_USER during the test in order to assert it's not set? Then if there would be any interference, this "precondition" assert would fail and it wouldn't go unnoticed. It's less complex than changing the env, and safer than current implementation. What do you think? Further, we could assert the PGUSER env is none or different from what we receive from the parsing, for example. Just throwing ideas |
|
I do like the thinking (and the idea of keeping it simple) but
unfortunately I don't think that fully addresses @abonander's concern
since, with tests running in parallel, another test could set/unset PGUSER
while ours is running (e.g., setting it after we check that it's unset but
before we run our actual logic). Global state is a nightmare.
It may work fine in practice (especially today where I don't think any
other tests ever set/unset PGUSER), but theoretically it's racy.
|
|
@svnrb Got it, you're right. So we'll wait on the @abonander direction and I'll fix this PR right up. (: |
|
Ideally, I think we add a CI step that runs the |
3 participants
Does your PR solve an issue?
Fixes #4277
Is this a breaking change?
No, just a bug fix.
Reference
This PR is just a rehash of #4275 from @svnrb.
I just re-opened this PR because he said he was gonna be away for 2 weeks and I have so time to maybe move this along!