-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[Bug] Expire session when update user password #15219
[Bug] Expire session when update user password #15219
Conversation
71b0e71
to
931f365
Compare
* session | ||
*/ | ||
@Builder | ||
@Data |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
Session.getIp
super(sessionMapper); | ||
} | ||
|
||
public void deleteByUserId(Integer userId) { |
Check notice
Code scanning / CodeQL
Missing Override annotation Note
SessionDao.deleteByUserId
b738456
to
14af4de
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #15219 +/- ##
============================================
- Coverage 38.00% 37.99% -0.02%
+ Complexity 4644 4640 -4
============================================
Files 1279 1279
Lines 44539 44490 -49
Branches 4800 4794 -6
============================================
- Hits 16927 16902 -25
+ Misses 25759 25733 -26
- Partials 1853 1855 +2 ☔ View full report in Codecov by Sentry. |
23e360d
to
f9a702b
Compare
// We will not bind session with ip | ||
@Deprecated |
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.
👍
|
||
@Override | ||
public List<Session> queryByUserId(Integer userId) { | ||
return mybatisMapper.selectList(new QueryWrapper<>(Session.builder().userId(userId).build())); |
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.
Can we avoid to use wrapper? Although this is a simple query looks fine, many complex query wrapper may be written later.
Use mapper.xml uniformly is a better way, WDYT?
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.
Agree with you, we shouldn't expose wrapper to service. I change this method to
/**
* Query the entity by condition.
*/
List<Entity> queryByCondition(Entity queryCondition);
And add implementation in BaseDao
@Override
public List<ENTITY> queryByCondition(ENTITY queryCondition) {
if (queryCondition == null) {
throw new IllegalArgumentException("queryCondition can not be null");
}
return mybatisMapper.selectList(new QueryWrapper<>(queryCondition));
}
Then we can avoid using wrapper in dao
, then we can use this method for simple query, and remove a lot of method like queryByCode
, queryByName
, queryByXX
, for this kind of query we can directly use dao.queryByCondition(entity).
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.
It seems good.
ae7522d
to
c09916c
Compare
c09916c
to
72c8885
Compare
Please retry analysis of this Pull-Request directly on SonarCloud. |
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.
LGTM
Purpose of the pull request
When we delete or update the user's password, the old session will expire.
Brief change log
Verify this pull request
This pull request is code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(or)
If your pull request contain incompatible change, you should also add it to
docs/docs/en/guide/upgrede/incompatible.md