src: db: introduce db interface #521

kwy95 · 2021-11-25T21:17:13Z

In an effort to standardize our data collection and storage, as well as
improve scalability this introduces a library to create and interact
with sqlite3 databases. This will eventually replace the files and
folders found in KW_DATA_DIR with a unified database.

Signed-off-by: Rubens Gomes Neto rubens.gomes.neto@usp.br

codecov-commenter · 2021-11-25T21:19:50Z

Codecov Report

Merging #521 (c9d484c) into unstable (e3c3e83) will increase coverage by 0.44%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##           unstable     #521      +/-   ##
============================================
+ Coverage     71.42%   71.87%   +0.44%     
============================================
  Files            31       32       +1     
  Lines          4451     4522      +71     
============================================
+ Hits           3179     3250      +71     
  Misses         1272     1272

Impacted Files	Coverage Δ
src/kw_db.sh	`100.00% <100.00%> (ø)`
src/kw_string.sh	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3c3e83...c9d484c. Read the comment docs.

rodrigosiqueira

Nice PR!
Could you break it in small set of commits followed by documentation and tests?
Thanks.

rodrigosiqueira · 2021-11-25T21:40:53Z

etc/db_files/tables.sql

+CREATE TABLE IF NOT EXISTS tags(
+	id INTEGER PRIMARY KEY,
+	tag VARCHAR(32) UNIQUE
+);
+
+CREATE TABLE IF NOT EXISTS pomodoro(
+	-- id INTEGER PRIMARY KEY,
+	tag_id INTEGER REFERENCES tags(id),
+	dt_start DATETIME NOT NULL,
+	dt_end DATETIME NOT NULL,
+	descript VARCHAR(512)
+);
+
+CREATE TABLE IF NOT EXISTS statistic(
+	-- id INTEGER PRIMARY KEY,
+	label VARCHAR(32),
+	dt_start DATETIME NOT NULL,
+	dt_end DATETIME NOT NULL
+);


I don't think we want to have this file here at this stage. We will need to discuss the data structure later... for now, you can move it to tests/samples/db_files and use it for test only.

src/kw_db.sh

rodrigosiqueira · 2021-11-25T21:42:38Z

src/kw_db.sh

+DB_NAME='kw.db'
+DB_PATH="$KW_ETC_DIR/db_files/$DB_NAME"


Let's use declare -g.

src/kw_db.sh

rodrigosiqueira · 2021-11-25T21:46:19Z

src/kw_db.sh

+function read_sql()
+{
+  local sql_path="$1"
+  local db="${2:-$DB_PATH}"
+
+  if [[ ! -f "$sql_path" ]]; then
+    printf '%s\n' "Could not find the file: $sql_path"
+    return 22
+  fi
+
+  sqlite3 "$db" < "$sql_path"
+}


Commit 1: Add support to create a database by using an SQL file.

Find a better name for this function

Validate each parameter

Add documentation

Add unit test

rodrigosiqueira · 2021-11-25T21:50:05Z

src/kw_db.sh

+function apply_command()
+{
+  local cmd="$1"
+  local db="${2:-$DB_PATH}"
+
+  sqlite3 "$db" -batch "$cmd"
+}


Commit 2: Add support to run SQL commands
See previous comments.

rodrigosiqueira · 2021-12-04T21:24:11Z

src/kwio.sh

@@ -172,3 +172,23 @@ function ask_with_default()

  printf '%s\n' "$response"
 }
+
+# Check if file exists and display an error message if supplied


How about something like this:
Check if the file exists, and if a message parameter is passed to this function, it will display that as an error message.

rodrigosiqueira · 2021-12-04T21:27:41Z

src/kw_db.sh

@@ -0,0 +1,30 @@
+# This file handles the interactions with the kw database
+# It should be used to read, write and modify data in kw's dbs


I think line 1 is enough here, and you can delete the second line.

rodrigosiqueira · 2021-12-04T21:30:40Z

src/kw_db.sh

+  file_exists "$sql_path" || exit_msg "Could not find the file: $sql_path" 2
+  file_exists "$db_path" || warning "Creating database: $db_path"


You introduced a message parameter to file_exists in the previous commit. Why not use it here? Actually, you can add an extra parameter to file_exists for force exit, what do you think?

I'm thinking of removing the message from that function and only check if the file exists and return accordingly. And now I think it might be easier to just use [[ -f "$path" ]]. What do you think?

rodrigosiqueira · 2021-12-04T21:31:37Z

src/kwio.sh

+{
+  local file_path="$1"
+  local message="$2"
+


how about adding one extra parameter to allow the developer to select the message type? For example, it would be nice to opt for a warning instead of complain message.

rodrigosiqueira · 2021-12-04T21:37:52Z

tests/kw_db_test.sh

+
+  assertTrue "($LINENO) DB file should be created" '[[ -f "$KW_DATA_DIR/kw.db" ]]'
+
+  output=$(sqlite3 "$KW_DATA_DIR/kw.db" -cmd '.tables' -batch ';')


Could you just add a comment or two that describes the sqlite command you executed? I mean, I don't know what I should expect from that.
Maybe add a sample of the expected output?

rodrigosiqueira · 2021-12-04T21:57:49Z

src/kw_db.sh

+    value="${value//\'/\'\'}" # escapes single quotes
+    values+="'$value'"


value and values sound confusing in the code. Maybe you can you something else instead of value.

rodrigosiqueira · 2021-12-04T21:58:36Z

src/kw_db.sh

+  local value
+  local count=0
+
+  [[ "$#" == 0 ]] && exit_msg 'No arguments given' 22


Do we want to exit here?

rodrigosiqueira · 2021-12-04T21:59:22Z

tests/kw_db_test.sh

+  assert_equals_helper 'No arguments, should fail' "$LINENO" 22 "$ret"
+  assert_equals_helper 'Expected error msg' "$LINENO" "$output" "$expected"
+
+  output=$(format_entries_db 'first' 'second' 'third')


How about adding a test with a single value?

rodrigosiqueira · 2021-12-04T22:00:08Z

tests/kw_db_test.sh

+  cd "$FAKE_DATA" || {
+    ret="$?"
+    fail "($LINENO): Failed to move to fake data dir"
+    exit "$ret"
+  }


Again, I would prefer that we do not cd this folder.

rodrigosiqueira · 2021-12-04T22:01:47Z

tests/kw_db_test.sh

+
+  output=$(format_values_db 1 "some 'quotes'")
+  ret="$?"
+  expected="('some ''quotes''')"


Don't we have one extra quote here?

Nope. There should be quotes around the full string, and then 1 extra quote for each quote inside the original string.

rodrigosiqueira · 2021-12-04T22:03:03Z

Btw, I don't think this is a draft anymore.

rodrigosiqueira · 2021-12-05T15:45:55Z

src/kw_db.sh

+  file_exists "$sql_path" || exit_msg "Could not find the file: $sql_path" 2
+  file_exists "$db_path" || warning "Creating database: $db_path"


rodrigosiqueira · 2021-12-05T15:49:25Z

src/kw_db.sh

+
+  file_exists "$db_path" || exit_msg 'Database does not exist' 2
+
+  sqlite3 "$db" -batch "$sql_cmd"


We should return something in case of a problem and let the function that invokes it handle the issue. However, I don't think we should add any warning message here because this is not something the user will understand in case of an error. Ideally, we should have the verbose or debug mode for handling these cases. (#179)

In an effort to standardize our data collection and storage, as well as improve scalability this starts a library to create and interact with `sqlite3` databases. This will eventually replace the files and folders found in `KW_DATA_DIR` with a unified database. This first commit creates a function `execute_sql` that executes a SQL script on a given database. Signed-off-by: Rubens Gomes Neto <rubens.gomes.neto@usp.br>

Introduce function to execute an SQL command in a given database. Signed-off-by: Rubens Gomes Neto <rubens.gomes.neto@usp.br>

Adds a function that adds values to a given table of a given database. Signed-off-by: Rubens Gomes Neto <rubens.gomes.neto@usp.br>

Adds `concatenate_with_commas` that returns the arguments as a comma separated list. Adds `format_values_db` that takes arguments and formats them to be used with commands like `insert_into`. Signed-off-by: Rubens Gomes Neto <rubens.gomes.neto@usp.br>

Adds a function to perform simple selects to any given table of a database. Signed-off-by: Rubens Gomes Neto <rubens.gomes.neto@usp.br>

rodrigosiqueira · 2021-12-07T12:15:42Z

Thanks a lot for this PR!
Merged to the unstable branch.

rodrigosiqueira requested changes Nov 25, 2021

View reviewed changes

kwy95 mentioned this pull request Dec 2, 2021

Report description doesn't work with commas #495

Closed

kwy95 force-pushed the sql-db branch 2 times, most recently from 829c111 to 15fc604 Compare December 4, 2021 00:19

rodrigosiqueira requested changes Dec 4, 2021

View reviewed changes

rodrigosiqueira requested changes Dec 5, 2021

View reviewed changes

kwy95 added 2 commits December 5, 2021 16:09

src: db: add execute_command

f2db1a5

Introduce function to execute an SQL command in a given database. Signed-off-by: Rubens Gomes Neto <rubens.gomes.neto@usp.br>

kwy95 force-pushed the sql-db branch from 15fc604 to 3f381b3 Compare December 5, 2021 20:11

kwy95 marked this pull request as ready for review December 5, 2021 20:11

kwy95 force-pushed the sql-db branch from 3f381b3 to 44b9b83 Compare December 6, 2021 17:56

kwy95 added 3 commits December 6, 2021 15:01

src: db: add insert_into function

012f135

Adds a function that adds values to a given table of a given database. Signed-off-by: Rubens Gomes Neto <rubens.gomes.neto@usp.br>

src: db: add select_from function

c9d484c

Adds a function to perform simple selects to any given table of a database. Signed-off-by: Rubens Gomes Neto <rubens.gomes.neto@usp.br>

kwy95 force-pushed the sql-db branch from 44b9b83 to c9d484c Compare December 6, 2021 18:01

rodrigosiqueira closed this Dec 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: db: introduce db interface #521

src: db: introduce db interface #521

kwy95 commented Nov 25, 2021

codecov-commenter commented Nov 25, 2021 •

edited

rodrigosiqueira left a comment

rodrigosiqueira Nov 25, 2021

rodrigosiqueira Nov 25, 2021

rodrigosiqueira Nov 25, 2021

rodrigosiqueira Nov 25, 2021

rodrigosiqueira Dec 4, 2021

rodrigosiqueira Dec 4, 2021

rodrigosiqueira Dec 4, 2021

kwy95 Dec 4, 2021

rodrigosiqueira Dec 5, 2021

rodrigosiqueira Dec 4, 2021

rodrigosiqueira Dec 4, 2021

rodrigosiqueira Dec 4, 2021

rodrigosiqueira Dec 4, 2021

rodrigosiqueira Dec 4, 2021

rodrigosiqueira Dec 4, 2021

rodrigosiqueira Dec 4, 2021

kwy95 Dec 5, 2021

rodrigosiqueira commented Dec 4, 2021

rodrigosiqueira Dec 5, 2021

rodrigosiqueira Dec 5, 2021

rodrigosiqueira commented Dec 7, 2021

		@@ -0,0 +1,30 @@
		# This file handles the interactions with the kw database
		# It should be used to read, write and modify data in kw's dbs

		file_exists "$sql_path" \|\| exit_msg "Could not find the file: $sql_path" 2
		file_exists "$db_path" \|\| warning "Creating database: $db_path"


		assertTrue "($LINENO) DB file should be created" '[[ -f "$KW_DATA_DIR/kw.db" ]]'

		output=$(sqlite3 "$KW_DATA_DIR/kw.db" -cmd '.tables' -batch ';')

		value="${value//\'/\'\'}" # escapes single quotes
		values+="'$value'"


		file_exists "$db_path" \|\| exit_msg 'Database does not exist' 2

		sqlite3 "$db" -batch "$sql_cmd"

		DB_NAME='kw.db'
		DB_PATH="$KW_ETC_DIR/db_files/$DB_NAME"

src: db: introduce db interface #521

src: db: introduce db interface #521

Conversation

kwy95 commented Nov 25, 2021

codecov-commenter commented Nov 25, 2021 • edited

Codecov Report

rodrigosiqueira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigosiqueira commented Dec 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigosiqueira commented Dec 7, 2021

codecov-commenter commented Nov 25, 2021 •

edited