Review my datastore module

Help and Feedback Code Review
#1

If anyone could comment on any potential data leaks or potential errors that could arise please let me know. Thanks!

local DsService = game:GetService("DataStoreService")
local GeneralDataStore = DsService:GetDataStore("V:1.0")

local DataModule = {}
local PlayerSessionData = {}
local DataNeededToBeFetched = {} 
local RetryInterval = 3

function CreatePlayerDataTable()
	local Data ={
		DataSave = 0,
		Coins = 0,
	}
	return Data
end

function DataModule:GetData(Player)
	local UserId = Player.UserId
	DataNeededToBeFetched[UserId] = true
	for i = 1,3,1 do
		print("Fetching data: "..Player.Name)
		local Data
		local Completed,Error = pcall(function()
			Data = GeneralDataStore:GetAsync(UserId)
		end)
		if Completed then
			if DataNeededToBeFetched[UserId] then
				if Data == nil then
					print(Player.Name.." is a new player.")
					Data = CreatePlayerDataTable()
				end
				DataNeededToBeFetched[UserId] = nil
				PlayerSessionData[UserId] = Data
				return
			else return
			end
		else
			print("Error fetching data: "..Player.Name.." Message: "..Error)
		end
		wait(RetryInterval)
	end
end

function DataModule:UpdateData(Player)
	local UserId = Player.UserId
	DataNeededToBeFetched[UserId] = nil
	if PlayerSessionData[UserId] then
		PlayerSessionData[UserId].DataSave = PlayerSessionData[UserId].DataSave + 1
		for i = 1,3,1 do
			print("Attempting to save data: "..Player.Name)
			local NewData = PlayerSessionData[UserId]
			local Completed,Error = pcall(function()
				GeneralDataStore:UpdateAsync(UserId,function(OldData)
					if OldData == nil or OldData.DataSave < NewData.DataSave then
						return NewData
					else
						print("Data save number of current session does not exceed previous session: "..Player.Name)
						return OldData
					end
				end)
			end)
			if Completed then
				print("Saved data")
				return
			else
				print("Error updating data: "..Player.Name.." Message: "..Error)
			end
			wait(RetryInterval)
		end
	else 
		print("Data for player does not yet exist")
	end
end

return DataModule
#2

Data leaks

I don’t suspect anything may be any more vulnerable than for (mostly) any other game, since nothing reaches the client. Of course, encrypting your values could help prevent against more advanced exploits (i.e. single-request attacks). Also, if you’re really big on security, you should avoid printing sensitive data.

Potential errors

I’m assuming this module is run on the server, and that the syntax is fully valid. From the UpdateData method, I see you’ve built in redundancies, such as the data-save number.

Additional comments

My only problem is: how will you be able to save data? Will the data table defined in CreatePlayerDataTable be modified elsewhere, then referenced again in the data storage module to have its values saved? If you’re doing it like that, kudos to you.

1 Like
#3

Going to pitch in some cents here.

First one is also for @VisualPlugin: if the data module is server-sided, security is a non-concern here. Printing is strongly encouraged for the sale of debugging but realistically it should not be present in a production build. I suggest a print wrapper that only prints when Studio is active.

local STUDIO_ACTIVE = game:GetService("RunService"):IsStudio()

local function debugprint(...)
    if STUDIO_ACTIVE then
        print(...)
    end
end

Your retry function is, well, pretty gross. There’s nested if statements and returns dropped everywhere. I recommend using repeat. Repeat loops, like while loops, run until a condition is met. The key difference is that with a repeat loop, one iteration is guaranteed to run. You can use this to your advantage.

local MAX_RETRIES = 3

-- Acceptable to have pcall returns as upvalues
-- Should wrap the below in a do-end block or function
local success = false
local retries = 0
local data

repeat
    success, data = pcall()

    -- If no success, wait before proceeding with conditional check and/or new iteration
    if not success then
        retries = retries + 1
        wait(1)
    end
until success or retries == 3

That aside, your code looks reasonably good to go.

1 Like
#4

I disagree with your first point. Someone could very well find a way to leak game data by accessing private APIs. Many hackers wouldn’t take the time to discover such a vulnerability, so they’d rely on exploiting the client instead, which is pretty secure at this point.

#5

If the module is server-sided, clients can’t access data or the methods, period. Printing data to the console serves no purpose other than for the sake of debugging and it’s not even visible (or replicated) to an unauthorised client.

Nothing sensitive is being printed in the first place and clients can’t do anything with printed items either unless your code structure is just that bad.

#6

I’m not saying we access the module itself. I’m saying that we access the DataStore API directly. If you’re familiar with the way HTTP APIs work, you’ll know that almost all requests can be forged elsewhere. I’m hypothesising that all data storage transactions rely on HTTP.

#7

Why does that matter in the slightest? You can’t access DataStores except throughout the API that Roblox provides and even then, it’s completely irrelevant to the issue or the statement that I pointed out. An exploiter cannot access a server-bound method, period.

Your suggestions are a little too far fetched over a print statement. Please ensure you know what you’re talking about before replying. A lot of your responses are based off of assumptions.

I would also like to ask that any further commentary be moved to a private message response to me. See the Code Review guidelines under “How should I reply to others?”:

1 Like