#19-dynamic-table-database #460
Labels
No labels
_CRITICAL_
API
app
backEnd
Blocked-waiting-for-further-changes
bug
bug-only-on-server-for-mobile-not-webpage
Bug-Report-After-Merge
cleanup
close
design
duplicate
enhancement
feature request
frontEnd
further-changes-needed
future-problem-not-fixint-this-period
help wanted
invalid
last-week-issue-to-fix
library
low-priority
needs input
needs review
not-implemented.
project documentation
question
research
reviewed
Script
security
SQL
style
Team 1
Team 2
team leaders
test-creation
testing
topLevel
unassigned
Under-review
wontfix
No milestone
No project
No assignees
6 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Andras/BoundlessFlowCampus2K!460
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "#19-dynamic-table-database"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Looks good with the dynamic table, it makes life easy for re usability later.
However I added a comment about the better error handling and we will have a meeting for clarifying the side effects of this pr and what needs to be done later.
Don't merge this pr until the update after the meeting.
@ -123,1 +169,4 @@string tablename = Sanitize(topic);var jpayload = JsonObject.Parse(payload);string dirtySqlDataTypes = jpayload[0].ToString();I would add try catch for InsertMessage, DeleteMessage and updateMessage for better error handling with relevent message like
`
try{
your logic here
}catch(){
Console.WriteLine($"Operation InserMessage from middleware, it failed with error message : {e.Message}");
throw;
}
`
`
try{
your logic here
}catch(){
Console.WriteLine($"Operation DeleteMessage from middleware, it failed with error message : {e.Message}");
throw;
}
`
try{
your logic here
}catch(){
Console.WriteLine($"Operation Update Message from middleware, it failed with error message : {e.Message}");
throw;
}
Done review from me! 👍
Just a questions for my own curiosity: What is the intent of the UpdateMessage and DeleteMessage? As far as I know, all current mockups only insert into their tables to keep history of values. But what purpose would an update and delete function serve in such a database?
like in roombooking you may want to change the date on an existing booking or delete some bookings.
So the idea is to simulate the booking process entirely. Got it. Just wondering if this is related to #43, as it only asks for visualization of booked rooms. Now it might be that Andras is a bit ambiguous in his wording and also means to add the ability to add, change and delete bookings. I assume that all mockup data is to be replaced with real data in the future (hopefully) and if that is the case we might put a lot of time on something that essentially will be replaced with the TimeEdit API.
I have spent some time trying to get the tempsensor to work with the new middleware took some time but got it to work.
The problem i had was i tried to use DUBLE(5,2) as a datatype but in the middleware it became an index problem as it saw the comma as the start of a new datatype.
I havent checked it completly as i was only working on getting the tempSensor to work but will try to look through the rest after a break.
@c24danli wrote in #460 (comment):
We had some work earlier that used an array to store the different SQL datatypes, but there are issues with trying to send an array through MQTT as the result is still just a string arriving at the destination.
I suppose changing the split function to use another character than ',' to split the datatype string could solve this issue, but we'll have to discuss which character would work best for this. I suspect a character not used by SQL-queries would be best.
A bit tierd so might be some mistakes but its more of list to talk about tomorrow
Rewiew
Strong points
Fixes/uncertainties
middleware\Program.cs
don’t mind it but wasn’t switch statements one of the nono’s that we had in software engenering?
switch (action)
{
case "insert":
await CreateTable(topic, dataBase, payload);
await InsertMessage(topic, dataBase, payload);
break;
case "update":
await UpdateMessage(topic, dataBase, payload);
break;
case "delete":
await DeleteMessage(topic, dataBase, payload);
break;
}
Old code left
line 206
// Old sql stringstring sql = $"INSERT INTO {tablename} (payload) VALUES (@payload)";line 212
cmd.Parameters.AddWithValue("@payload", payload);line 243
cmd.Parameters.AddWithValue("@payload", payload);line 292
cmd.Parameters.AddWithValue("@payload", payload);Safety
not safe for sql injections not a big deal for the data from mockup as we control it, not sure how it will work in parking/bookingroom but if the user can input values say regnumber for car harmful sql injections could be a thing.
example code:
secondhalfsql += $"'{jpayload[i + 2]}'";if example regnr is inserted by the user
ABC123'; DROP TABLE t_sensors_...could be a thing. Probably could be handled in frontend but should it?Uppdate
Later problem but updates becomes a bit more complex as we will need more data then normally, normally primarykey, tablename and column to chage with value is needed but now we will need sqlDataTypes with matching amount to the columns that are changed or send a complete row.
Redability/
because it uses index/positions instead of names it can become hard to understand intuativly ex:
string secondhalfsql = $" WHERE {jpayload[1].GetPropertyName()} = '{jpayload[1]}';"Or when looping through “Jpayload” could not
be used instead of
for (int i = 0; i < sqlDataTypes.Length; i++)As its an Json object with keys and values?
If it works then the problem I had with inserting DECIMAL(5,2) would not be a problem as there would be no need for the
string.Split(",").Very detailed review, this will be merged into the team branch.