DataStore Review

Help and Feedback Code Review
1

Hi there is my datastore script good enough or do i need to change things for better optimization or make it more secure.

local Players = game:GetService("Players")
local DataService = game:GetService("DataStoreService")
local HttpsService = game:GetService("HttpService")

local MainDataStore = DataService:GetDataStore("MainData")
local SessionStore = DataService:GetDataStore("SessionStore")

local MaterialData = require(script.Parent.MaterialData)
local DataStoreTest = require(script.DataStoreTestSuite)

local ServerSessionId = game.JobId ~= "" and game.JobId or HttpsService:GenerateGUID(false)
local SESSION_TIMEOUT = 300


local DataHandler = {}
DataHandler.__index = DataHandler

local GlobalData = {}
local SaveLock = {}

local function ReleaseSessionLock(plr: Player)
	local userId = tostring(plr.UserId)
	local sessionKey = "Session_" .. tostring(userId)
	pcall(function()
		local current = SessionStore:GetAsync(sessionKey)
		if current and current.SessionId == ServerSessionId then
			SessionStore:RemoveAsync(sessionKey)
		end
	end)
end

function DataHandler.InitData(plr:Player)
	local userId = tostring(plr.UserId)
	local sessionKey = "Session_" .. tostring(userId)
	local now = os.time()
	print(now)
	
	local got
	
	local succes,error = pcall(function()
		 got = SessionStore:UpdateAsync(sessionKey, function(current)
			if current == nil then
				warn("nothing")
				return {
					SessionId = ServerSessionId,
					TimeStamp = now
				}
			end
			
			if current.SessionId == ServerSessionId then
				warn("Same Server")
				return current
			end
			
			if now - (current.TimeStamp) >SESSION_TIMEOUT then
				warn("STAKE LOCK DETECTED FOR "..plr.Name.." Overiding")
				return {
					SessionId = ServerSessionId,
					TimeStamp = now
				}
			end
			warn("returing nil")
		return nil	
		end)
	end)
	
	if not succes or not got then
		plr:Kick("Your Data is locked by another session")
		return
	end
	
	
	local data
	local s,e = pcall(function()
		data = MainDataStore:GetAsync(tostring(plr.UserId))
	end)
	if s then
		if data then
			print("Succesfully loaded")
			data = DataHandler.ValidateData(data)
			GlobalData[plr.UserId] = data
			print(GlobalData[plr.UserId])
		else
			print("Doesnt have data")
			GlobalData[plr.UserId] = DataHandler.ValidateData({})
		end
	else
		warn("Couldnt Load data "..tostring(e))
		plr:Kick("Couldnt Load Data")
	end
end

function DataHandler.ValidateData(data)
	if type(data) ~= "table" then return {Inventory = {}, DataVersion = 0} end
	data.Inventory = typeof(data.Inventory) == "table" and data.Inventory or {}
	data.DataVersion = typeof(data.DataVersion) == "number" and data.DataVersion or 0
	return data
end

function DataHandler.SaveData(plr:Player)
	if SaveLock[plr.UserId] == true then return end
	SaveLock[plr.UserId] = true
	local start = tick()
	local s,e = pcall(function()
		MainDataStore:UpdateAsync(tostring(plr.UserId),function(old)
			if GlobalData[plr.UserId] == nil then return end
			if type(GlobalData[plr.UserId].Inventory) ~= "table" or typeof(GlobalData[plr.UserId].DataVersion) ~= "number" then
				warn("Invalid data format for ",plr)
				GlobalData[plr.UserId] = DataHandler.ValidateData(GlobalData[plr.UserId])
				print(GlobalData[plr.UserId])
				return GlobalData[plr.UserId]
			end
			
			if not GlobalData[plr.UserId] then
				warn("No data found for", plr)
				GlobalData[plr.UserId] = DataHandler.ValidateData({})
				return GlobalData[plr.UserId]
			end
			
			if (old) then
				if old.DataVersion ~= GlobalData[plr.UserId].DataVersion then
					return nil
				else
					GlobalData[plr.UserId].DataVersion += 1
						print("Saved", plr.Name, "in", tick() - start, "seconds")
					return GlobalData[plr.UserId]
				end
			else
				GlobalData[plr.UserId].DataVersion += 1
				print("Saved", plr.Name, "in", tick() - start, "seconds")
				return GlobalData[plr.UserId]
			end
		end)
	end)
	if s then
		print("Succesfully Saved Data")
		SaveLock[plr.UserId] = nil
	else
		task.delay(1, function()
			local retrySuccess = pcall(function()
				DataHandler.SaveData(plr)
			end)
			if not retrySuccess then
				plr:Kick("Could not autosave after retry: "..tostring(e))
			end
		end)

	end
end

function DataHandler.GetPlayerData(plr)
	local copy = table.clone(GlobalData[plr.UserId])
	return copy
end

function DataHandler.SetPlayerInventoryData(plr, newData)
	GlobalData[plr.UserId].Inventory = newData
end

Players.PlayerRemoving:Connect(function(plr)
	warn("Using playeremoving")
	ReleaseSessionLock(plr)
	DataHandler.SaveData(plr)
	GlobalData[plr.UserId] = nil
end)
	

game:BindToClose(function()
	warn("Using BindToclose")
	for _,plr in pairs(Players:GetPlayers()) do
		coroutine.resume(coroutine.create(function()
			ReleaseSessionLock(plr)
			local s,e = pcall(function()
				if not plr or not plr:IsDescendantOf(game) then return end
				DataHandler.SaveData(plr)
			end)
			if not s then
				warn(tostring(e))
			end
		end))()
	end
end)

coroutine.resume(coroutine.create(function()
	while true do
		for _,plr in pairs(Players:GetPlayers()) do
			if GlobalData[plr.UserId] then
				coroutine.wrap(DataHandler.SaveData)(plr)
			end
		end
		task.wait(60)
	end	
end))
return DataHandler
2

Cool stuff with the session locking system.

Improvements to Readability

Change succes → success (also in prints)

Instead of if SaveLock[plr.UserId] == true, just do if SaveLock[plr.UserId], less work. Same for any if-statements like this. If the opposite, use if not .... Opposites include if ... == nil for example.

You can spawn functions rather than relying on coroutines such as that coroutine.wrap, that will make it cleaner. task.spawn?

You can change this to be if s and data then to reduce nesting. Even better, use a guard clause which means you return if it doesn’t meet the condition. There’s nothing after this if-statement anyways.

if s then
		if data then
			print("Succesfully loaded")
			data = DataHandler.ValidateData(data)
			GlobalData[plr.UserId] = data
			print(GlobalData[plr.UserId])
		else
			print("Doesnt have data")
			GlobalData[plr.UserId] = DataHandler.ValidateData({})
		end
	else
		warn("Couldnt Load data "..tostring(e))
		plr:Kick("Couldnt Load Data")
	end
-- ...

Bad names: local s,e. Use success, result.

Useless Code

if not plr or not plr:IsDescendantOf(game) then return end, you are iterating through Players:GetPlayers() which you are guaranteed that every object you are looking at, is indeed a Player instance. Therefore, the Player instance can always be referenced. Even when the object is destroyed, the reference will still be there. It’s fair enough to just use :IsDescendantOf.

You are being redundant, you are already going to return an empty inventory and 0 version even if you didn’t have the if-statement in the beginning.

function DataHandler.ValidateData(data)
	if type(data) ~= "table" then return {Inventory = {}, DataVersion = 0} end -- remove this line
	data.Inventory = typeof(data.Inventory) == "table" and data.Inventory or {}
	data.DataVersion = typeof(data.DataVersion) == "number" and data.DataVersion or 0
	return data
end

Redundancy:
userId is already of type string after the tostring call, you don’t need to call tostring on it again in the sessionKey declaration.

local userId = tostring(plr.UserId)
local sessionKey = "Session_" .. tostring(userId)

What is the point of __index? I thought you might have been trying to do OOP but I see no constructor like .new.

There’s a chance that a DataStore API request fails because Roblox failed to get the player’s data, rather than there being a session. You might want to do a limited amount of retries in this case rather than kicking the player out.

Why have a versioning sanity check system for player data if you have session locking as a security mechanism? Feel free to explain to me how you’re using both systems together. It’s not easy to tell from reading the code.

1 Like
3

Good job with the session lock! To make it better, try loading data a few times before kicking the player, explain why you use both version checks and session locks, and use false instead of nil to show when saving is done.

1 Like
4

The reason i use both versions check and server lock is because the versions check i dont want different data to be saved. I think its usefel if i have both because it add more layers of security to my datastore. And thank you for the review i will use the advice you gave me to update my script