Repository: danvergara/dblab
Pull request #145 (2023-02-12 18:42:56)

Fix limit validation
# Pull Request Template

## Description

I realized that the limit had to be a `uint` variable because makes no sense to give the possibility to the user to enter a negative value, so I changed its type. I'll let the runtime error out when a user enters a value lesser than zero.

## Type of change

Please delete options that are not relevant.

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [x] This change requires a documentation update

## How Has This Been Tested?

* Updated the current tests suite when needed
* I introduced negatives values through the config file
* I typed in a negative value through form app

## Checklist:

- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [x] I have made corresponding changes to the documentation
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [x] I have checked my code and corrected any misspellings



Patch:
diff --git a/README.md b/README.md
index e4dec3d..18a87d9 100644
--- a/README.md
+++ b/README.md
@@ -98,7 +98,7 @@ Flags:
       --driver string   Database driver
   -h, --help            help for dblab
       --host string     Server host name or IP
-      --limit int       Size of the result set from the table content query (default 100)
+      --limit int       Size of the result set from the table content query (should be greater than zero, otherwise the app will error out) (default 100)
       --pass string     Password for user
       --port string     Server port
       --schema string   Database schema (postgres only)
@@ -173,6 +173,7 @@ database:
     user: "postgres"
     schema: "public"
     driver: "postgres"
+# should be greater than 0, otherwise the app will error out
 limit: 50
 ```
 
diff --git a/cmd/root.go b/cmd/root.go
index 13ffcf6..3ad8b81 100644
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -23,7 +23,7 @@ var (
 	schema  string
 	db      string
 	ssl     string
-	limit   int
+	limit   uint
 )
 
 // NewRootCmd returns the root command.
@@ -117,5 +117,5 @@ func init() {
 	rootCmd.Flags().StringVarP(&db, "db", "", "", "Database name")
 	rootCmd.Flags().StringVarP(&schema, "schema", "", "", "Database schema (postgres only)")
 	rootCmd.Flags().StringVarP(&ssl, "ssl", "", "", "SSL mode")
-	rootCmd.Flags().IntVarP(&limit, "limit", "", 100, "Size of the result set from the table content query")
+	rootCmd.Flags().UintVarP(&limit, "limit", "", 100, "Size of the result set from the table content query (should be greater than zero, otherwise the app will error out)")
 }
diff --git a/pkg/client/client.go b/pkg/client/client.go
index 27a4f21..e046bb7 100644
--- a/pkg/client/client.go
+++ b/pkg/client/client.go
@@ -23,7 +23,7 @@ type Client struct {
 	db                *sqlx.DB
 	driver, schema    string
 	paginationManager *pagination.Manager
-	limit             int
+	limit             uint
 }
 
 // New return an instance of the client.
diff --git a/pkg/command/command.go b/pkg/command/command.go
index a73979e..b41506a 100644
--- a/pkg/command/command.go
+++ b/pkg/command/command.go
@@ -14,7 +14,7 @@ type Options struct {
 	SSL    string
 	// PostgreSQL only.
 	Schema string
-	Limit  int
+	Limit  uint
 }
 
 // SetDefault returns a Options struct and fills the empty
diff --git a/pkg/config/config.go b/pkg/config/config.go
index 8d9766b..0a84e32 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -30,7 +30,7 @@ type Config struct {
 	Port     string
 	DBName   string
 	Driver   string
-	Limit    int `fig:"limit" default:"100"`
+	Limit    uint `fig:"limit" default:"100"`
 }
 
 type Database struct {
diff --git a/pkg/form/form.go b/pkg/form/form.go
index 5a8bafa..7a6efd4 100644
--- a/pkg/form/form.go
+++ b/pkg/form/form.go
@@ -117,17 +117,17 @@ func (m *Model) SSL() string {
 }
 
 // Limit returns the limit input value from the user.
-func (m *Model) Limit() int {
+func (m *Model) Limit() (uint, error) {
 	limit, err := strconv.Atoi(m.limitInput.Value())
 	if err != nil {
-		return 100
+		return uint(0), err
 	}
 
 	if limit <= 0 {
-		return 100
+		return uint(0), fmt.Errorf("invalid limit %d", limit)
 	}
 
-	return limit
+	return uint(limit), nil
 }
 
 // FilePath returns the path to the database file (just in sqlite3) value.
@@ -207,6 +207,11 @@ func Run() (command.Options, error) {
 		return command.Options{}, err
 	}
 
+	limit, err := m.Limit()
+	if err != nil {
+		return command.Options{}, err
+	}
+
 	opts := command.Options{
 		Driver: m.driver,
 		Host:   m.Host(),
@@ -215,7 +220,7 @@ func Run() (command.Options, error) {
 		Pass:   m.Password(),
 		DBName: m.Database(),
 		SSL:    m.SSL(),
-		Limit:  m.Limit(),
+		Limit:  limit,
 	}
 
 	if m.driver == "sqlite3" {
diff --git a/pkg/pagination/pagination.go b/pkg/pagination/pagination.go
index 237bcb5..4ebfc0e 100644
--- a/pkg/pagination/pagination.go
+++ b/pkg/pagination/pagination.go
@@ -9,13 +9,13 @@ import (
 type Manager struct {
 	totalPages   int
 	currentPage  int
-	limit        int
+	limit        uint
 	offset       int
 	currentTable string
 }
 
 // New returns a pointer to a Manager instance.
-func New(limit, count int, currentTable string) (*Manager, error) {
+func New(limit uint, count int, currentTable string) (*Manager, error) {
 	m := Manager{
 		limit:        limit,
 		currentPage:  1,
@@ -61,7 +61,7 @@ func (m *Manager) Offset() int {
 }
 
 // Limit returns the limit.
-func (m *Manager) Limit() int {
+func (m *Manager) Limit() uint {
 	return m.limit
 }
 
@@ -87,16 +87,11 @@ func (m *Manager) CurrentTable() string {
 
 // setOffset calculates the offset based of the current page and the limit.
 func (m *Manager) setOffset() {
-	m.offset = (m.currentPage - 1) * m.limit
+	m.offset = (m.currentPage - 1) * int(m.limit)
 }
 
 // setTotalPages total pages = count / limit, if the limit is greater than 0.
 func (m *Manager) setTotalPages(count int) error {
-	// limit must be greater than 0.
-	if m.limit <= 0 {
-		return fmt.Errorf("limit should be greater than 0")
-	}
-
 	m.totalPages = int(math.Ceil(float64(count) / float64(m.limit)))
 
 	return nil


Test patch:
diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go
index 958b5f3..33a0a4f 100644
--- a/pkg/client/client_test.go
+++ b/pkg/client/client_test.go
@@ -173,7 +173,7 @@ func TestTableContent(t *testing.T) {
 
 	r, co, err := c.tableContent("products")
 
-	assert.Len(t, r, opts.Limit)
+	assert.Len(t, r, int(opts.Limit))
 	assert.Len(t, co, 3)
 	assert.NoError(t, err)
 }
@@ -349,7 +349,7 @@ func TestMetadata(t *testing.T) {
 	assert.Len(t, m.Structure.Columns, 8)
 
 	// table content.
-	assert.Len(t, m.TableContent.Rows, opts.Limit)
+	assert.Len(t, m.TableContent.Rows, int(opts.Limit))
 	assert.Len(t, m.TableContent.Columns, 3)
 }
 
diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
index ae3eca2..cb83a31 100644
--- a/pkg/config/config_test.go
+++ b/pkg/config/config_test.go
@@ -16,7 +16,7 @@ func TestInit(t *testing.T) {
 		pass   string
 		driver string
 		schema string
-		limit  int
+		limit  uint
 	}
 	var tests = []struct {
 		name  string
diff --git a/pkg/pagination/pagination_test.go b/pkg/pagination/pagination_test.go
index 388c490..1fd3f65 100644
--- a/pkg/pagination/pagination_test.go
+++ b/pkg/pagination/pagination_test.go
@@ -9,7 +9,7 @@ import (
 func TestPaginationLifeCycle(t *testing.T) {
 	type given struct {
 		count     int
-		limit     int
+		limit     uint
 		tableName string
 	}
 
@@ -82,7 +82,7 @@ func TestPaginationLifeCycle(t *testing.T) {
 			}
 
 			assert.Equal(t, tc.expected.lastPage, m.CurrentPage())
-			assert.Equal(t, (m.CurrentPage()-1)*tc.given.limit, m.Offset())
+			assert.Equal(t, (m.CurrentPage()-1)*int(tc.given.limit), m.Offset())
 
 			err = m.NextPage()
 			assert.Error(t, err)