Skip to content
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

Fixed out-of-range column lognum in the table api_user #3480

Merged
merged 2 commits into from
Aug 31, 2023

Conversation

kiatng
Copy link
Contributor

@kiatng kiatng commented Aug 30, 2023

Description (*)

The column lognum was type smallint, which has a limit of 65,535.

->addColumn('lognum', Varien_Db_Ddl_Table::TYPE_SMALLINT, null, [

Production:
image

Questions or comments

I'm not sure what's the use of lognum, especially when it hits the limit, it's useless. Nonetheless, it needs fixing.

@github-actions github-actions bot added the Component: Api PageRelates to Mage_Api label Aug 30, 2023
…6.0.3.php

Co-authored-by: Fabrizio Balliano <fabrizio.balliano@gmail.com>
@fballiano
Copy link
Contributor

wow, more than 65.000 logins?

@kiatng
Copy link
Contributor Author

kiatng commented Aug 30, 2023

wow, more than 65.000 logins?

Yes, not making this up, and probably has been out of range for ages.

@ADDISON74
Copy link
Contributor

1 login/hour x 24h/day x 30d/month x 12m/year = 8640.

In 10 years, if he does this, he can exceed a smallint. I reported to one hour because that's how long a session could last in general. If he logs out after 10 minutes and comes back in, then the number can increase. The observation is good because there can be several users on a single account.

@fballiano
Copy link
Contributor

dunno, I think it's a bit overkill, I think that the original idea is to check if a user does too many logins in order to take actions, in that case a smallint is fine.

anyway, not a big deal in any cases.

@fballiano
Copy link
Contributor

I hoped to get some other opinion but ok, let's merge

@fballiano fballiano merged commit 046450a into OpenMage:main Aug 31, 2023
15 checks passed
@ADDISON74
Copy link
Contributor

dunno, I think it's a bit overkill, I think that the original idea is to check if a user does too many logins in order to take actions, in that case a smallint is fine.

Changing the type of a column does not have major effects, maybe only on restoring the table indexes (I have not checked if this table has such a thing). I don't think more could be done in this PR. In order to take some actions against a user who logs too many times, you have to decide if it is a normal attempt or a fake one. If it fake the you can skip increasing to the number, but there were still chances to cross the maximum threshold of the value of the initial type. The probability of it happening is at least 1 in 10,000 stores.

@m-overlund
Copy link
Contributor

our api users (for Klaviyo and WMS) has maxed out after only a year or so.

@ADDISON74
Copy link
Contributor

our api users (for Klaviyo and WMS) has maxed out after only a year or so

In this case the PR will be useful for them and others in this situation.

@kiatng kiatng deleted the lognum_overflow branch September 5, 2023 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Api PageRelates to Mage_Api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants