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

Add Creation/LastLogin IP/DateTime #1985

Merged
merged 15 commits into from
Jul 6, 2019

Conversation

LtRipley36706
Copy link
Member

No description provided.

@Mag-nus
Copy link
Member

Mag-nus commented Jun 10, 2019

See my comments in this PR regarding storing IP addresses in a database: #1471

@Mag-nus
Copy link
Member

Mag-nus commented Jun 10, 2019

I would also make the following adjustments
email_Address should be nullable since it's a string.

create/last login IP should be the varbinary as suggested in the other PR, as well as nullable.

isBanned should be not nullable, default 0.

should ban_Reason also be stored in this table?

it might make sense to pull the ban columns out of account and maybe put them in a ban table and foreign link it to account.

@TheKnobGoblin
Copy link

See my comments in this PR regarding storing IP addresses in a database: #1471

I strongly disagree with storing the IP fields in VARBINARY. I've never seen it used that way and it majorly over complicates things. VARCHAR works perfectly fine for any IP address and it's used to store an IP 99% of the time for servers.

@Mag-nus
Copy link
Member

Mag-nus commented Jun 10, 2019

VARCHAR can be more convenient in certain scenarios, sure. If you want human readable values when you browse the table in a tool such as phpMyAdmin or MySQL Workbench, VARCHAR offers you this over VARBINARY(16)

However, VARBINARY(16) is the standard for storing ip addresses. It is more efficient for both storage and comparisons. VARBINARY(16) supports both IPV4 and IPV6 addresses. If we only want to store IPV4, then INT is the ideal column type. We can always change the column from INT to VARBINARY(16) in the future (if needed) and retain the existing values. I prefer using VARBINARY(16) though because it's the modern standard for ip storage.

What is the use case where you would prefer these values stored as VARCHAR?

@TheKnobGoblin
Copy link

The use case is that it makes both storing and reading the IP address easier. The performance gain from VARBINARY is negligible and makes the IP address unreadable when you want to simply look at the account table and see which IP belongs to which account.

I think it's silly to block this PR because it's not modern enough for you. Keeping things simple is the best approach, especially for a community driven open source emulator project.

@Mag-nus
Copy link
Member

Mag-nus commented Jun 10, 2019

I'm not blocking it, just offering suggestions.

I have some more comments, i'll bring them up in the development channel in ACE.

Mag-nus
Mag-nus previously approved these changes Jun 12, 2019
Copy link
Member

@Mag-nus Mag-nus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve but Ripley should also review/approve.

Mag-nus
Mag-nus previously approved these changes Jun 13, 2019
@LtRipley36706
Copy link
Member Author

I think this current state will meet both needs well. @TheKnobGoblin do the added fields that automatically convert to readable varchars work for you?

@LtRipley36706 LtRipley36706 merged commit 3fc219e into ACEmulator:master Jul 6, 2019
@LtRipley36706 LtRipley36706 deleted the authban branch July 29, 2019 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants