Repository: prest/prest (go)

Issue:
Different JOIN behaviour after prest upgrade
**Describe the bug**
Performing the same query when joining tables on different versions of prestd (with the same database backend) produces different outputs.

- prestd 1.0.5 -> OK
- prestd 1.0.14 -> NOK

**To Reproduce**
Steps to reproduce the behavior:
1. Request to endpoint `scans?_join=inner:bodyfat:scans.scancode:$eq:bodyfat.scancode&scans.scancode=$in.000005256e0dc77adbe8f6b25b75eda6748f0818&bodyfat.method=tina`
2. Attached the CSV file of both output as well as the XLS file for comparison. In the XLS I marked the differences with red. As you see we have different results for the same column `scans.gender` and also for the number of returned columns.

**Expected behavior**
Same and correct query results in both cases.

**Desktop (please complete the following information):**
 - pREST version (or commit ref): as written above
 - pREST endpoint: as written above
 - PostgreSQL version: 14.x
 - OS: [e.g. Debian Tid]: Both API running in Kubernetes Popds
 - Go version: NA
 - Log gist: NA

**Additional context**
Database table structure:

```
                                     Table "public.scans"
    Column     |          Type          | Collation | Nullable |           Default            
---------------+------------------------+-----------+----------+------------------------------
 scancode      | character varying(255) |           | not null | 
 creation_date | bigint                 |           | not null | 
 synthetic     | boolean                |           | not null | 
 creator       | character varying(50)  |           | not null | 
 gender        | character varying(50)  |           | not null | 'neutral'::character varying
 clothes       | boolean                |           |          | 
 fists         | boolean                |           |          | 
 pose          | character varying(50)  |           |          | 
 motion        | character varying(50)  |           |          | 
 scene         | character varying(50)  |           |          | 
 raw_data      | character varying(50)  |           |          | 
 user_id       | character varying(100) |           |          | 
 device_id     | character varying(100) |           |          | 
 age           | bigint                 |           |          | 
 weight        | double precision       |           |          | 
 height        | double precision       |           |          | 
 region        | character varying(50)  |           |          | 
 country       | character varying(50)  |           |          | 
 mocap         | boolean                |           | not null | false
 avatar        | character varying(50)  |           |          | 
 campaign      | character varying(255) |           |          | 
 ref_scan      | character varying(255) |           |          | 
 artefacts     | boolean                |           |          | 
 bad_hair      | boolean                |           |          | 
 bad_feet      | boolean                |           |          | 
 verified      | boolean                |           | not null | false
 ref_motion    | character varying(255) |           |          | 
Indexes:
    "scans_pkey" PRIMARY KEY, btree (scancode) 
```

```
                                     Table "public.bodyfat"
     Column      |          Type          | Collation | Nullable |           Default            
-----------------+------------------------+-----------+----------+------------------------------
 bodyfat_id      | integer                |           | not null | generated always as identity
 scancode        | character varying(255) |           | not null | 
 creation_date   | bigint                 |           | not null | 
 percentage      | numeric(7,2)           |           | not null | 
 method          | character varying(50)  |           | not null | 
 device          | character varying(50)  |           |          | 
 meta_cv_release | character varying(50)  |           |          | 
 fitting_id      | integer                |           |          | 
 gender          | character varying(50)  |           |          | 
Indexes:
    "bodyfat_pkey" PRIMARY KEY, btree (bodyfat_id)
    "bf_creation_date_idx" btree (creation_date)
    "bf_meta_cv_release_idx" btree (meta_cv_release)
    "bf_method_idx" btree (method)
    "bf_scancode_idx" btree (scancode)
Foreign-key constraints:
    "fk_fitting_id" FOREIGN KEY (fitting_id) REFERENCES model_fitting_results(fitting_id) ON DELETE CASCADE
    "fk_scancode" FOREIGN KEY (scancode) REFERENCES scans(scancode) ON DELETE CASCADE
```

Cc: @manhofer
[query_result_comparison.xlsx](https://github.com/prest/prest/files/10154654/query_result_comparison.xlsx)
[WORKING_db_query_result.csv](https://github.com/prest/prest/files/10154655/WORKING_db_query_result.csv)
[NOT_WORKING_db_query_result.csv](https://github.com/prest/prest/files/10154656/NOT_WORKING_db_query_result.csv)



Pull request:
Issue 746
This is to fix #746, before merging it should include:
- [x] docs
- [x] allow users to setup the JSON agg type with the `prest.toml` file
- [x] allow users to setup the JSON agg type with the `PREST_JSON_AGG_TYPE` env
- [x] tests

Other not related changes:
- [x] replaces the `context.Background()` with cobra's builtin `Context`.


<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

- **New Features**
  - Introduced flexibility in SQL query aggregation functions through configuration settings.

- **Improvements**
  - Updated command-line tools to use the command's context for migration operations, enhancing the consistency and control over command execution.

- **Refactor**
  - Consolidated configuration loading and parsing functions for improved code maintainability.

- **Tests**
  - Added new test cases to verify the correct assignment of JSON aggregation type in the configuration.

- **Documentation**
  - Updated the version number to reflect new changes in the application.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Patch:
diff --git a/adapters/postgres/postgres.go b/adapters/postgres/postgres.go
index 67985b4..5300906 100644
--- a/adapters/postgres/postgres.go
+++ b/adapters/postgres/postgres.go
@@ -666,7 +666,7 @@ func (adapter *Postgres) QueryCtx(ctx context.Context, SQL string, params ...int
 		log.Errorln(err)
 		return &scanner.PrestScanner{Error: err}
 	}
-	SQL = fmt.Sprintf("SELECT jsonb_agg(s) FROM (%s) s", SQL)
+	SQL = fmt.Sprintf("SELECT %s(s) FROM (%s) s", config.PrestConf.JSONAggType, SQL)
 	log.Debugln("generated SQL:", SQL, " parameters: ", params)
 	p, err := Prepare(db, SQL)
 	if err != nil {
@@ -691,7 +691,7 @@ func (adapter *Postgres) Query(SQL string, params ...interface{}) (sc adapters.S
 		log.Println(err)
 		return &scanner.PrestScanner{Error: err}
 	}
-	SQL = fmt.Sprintf("SELECT jsonb_agg(s) FROM (%s) s", SQL)
+	SQL = fmt.Sprintf("SELECT %s(s) FROM (%s) s", config.PrestConf.JSONAggType, SQL)
 	log.Debugln("generated SQL:", SQL, " parameters: ", params)
 	p, err := Prepare(db, SQL)
 	if err != nil {
diff --git a/cmd/down.go b/cmd/down.go
index f4fad48..1cdf6f5 100644
--- a/cmd/down.go
+++ b/cmd/down.go
@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"context"
 	"fmt"
 	"os"
 
@@ -16,7 +15,7 @@ var downCmd = &cobra.Command{
 	Long:    `Roll back all migrations`,
 	PreRunE: checkTable,
 	RunE: func(cmd *cobra.Command, args []string) error {
-		n, executed, err := migration.Run(context.Background(), path, urlConn, "down")
+		n, executed, err := migration.Run(cmd.Context(), path, urlConn, "down")
 		if err != nil {
 			return err
 		}
diff --git a/cmd/mversion.go b/cmd/mversion.go
index b238e0b..74834c3 100644
--- a/cmd/mversion.go
+++ b/cmd/mversion.go
@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"context"
 	"fmt"
 	"os"
 
@@ -16,7 +15,7 @@ var mversionCmd = &cobra.Command{
 	Long:    `Show the current migration version`,
 	PreRunE: checkTable,
 	RunE: func(cmd *cobra.Command, args []string) error {
-		n, executed, err := migration.Run(context.Background(), path, urlConn, "status")
+		n, executed, err := migration.Run(cmd.Context(), path, urlConn, "status")
 		if err != nil {
 			return err
 		}
diff --git a/cmd/next.go b/cmd/next.go
index c15d039..5cca85a 100644
--- a/cmd/next.go
+++ b/cmd/next.go
@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"context"
 	"fmt"
 	"os"
 	"strings"
@@ -27,9 +26,9 @@ var nextCmd = &cobra.Command{
 		)
 		a := args[0]
 		if strings.HasPrefix(a, "+") {
-			n, executed, err = migration.Run(context.Background(), path, urlConn, "up "+strings.TrimPrefix(a, "+"))
+			n, executed, err = migration.Run(cmd.Context(), path, urlConn, "up "+strings.TrimPrefix(a, "+"))
 		} else {
-			n, executed, err = migration.Run(context.Background(), path, urlConn, "down "+strings.TrimPrefix(a, "-"))
+			n, executed, err = migration.Run(cmd.Context(), path, urlConn, "down "+strings.TrimPrefix(a, "-"))
 		}
 		if err != nil {
 			return err
diff --git a/cmd/redo.go b/cmd/redo.go
index 47adff2..d894d47 100644
--- a/cmd/redo.go
+++ b/cmd/redo.go
@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"context"
 	"fmt"
 	"os"
 
@@ -16,7 +15,7 @@ var redoCmd = &cobra.Command{
 	Long:    `roll back the most recently applied migration, then run it again.`,
 	PreRunE: checkTable,
 	RunE: func(cmd *cobra.Command, args []string) error {
-		n, executed, err := migration.Run(context.Background(), path, urlConn, "down 1")
+		n, executed, err := migration.Run(cmd.Context(), path, urlConn, "down 1")
 		if err != nil {
 			return err
 		}
@@ -25,7 +24,7 @@ var redoCmd = &cobra.Command{
 		for _, e := range executed {
 			fmt.Fprintf(os.Stdout, "%v SUCCESS\n", e)
 		}
-		n, executed, err = migration.Run(context.Background(), path, urlConn, "up")
+		n, executed, err = migration.Run(cmd.Context(), path, urlConn, "up")
 		if err != nil {
 			return err
 		}
diff --git a/cmd/reset.go b/cmd/reset.go
index 8721134..d0ea191 100644
--- a/cmd/reset.go
+++ b/cmd/reset.go
@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"context"
 	"fmt"
 	"os"
 
@@ -16,7 +15,7 @@ var resetCmd = &cobra.Command{
 	Long:    `Run down and then up command`,
 	PreRunE: checkTable,
 	RunE: func(cmd *cobra.Command, args []string) error {
-		n, executed, err := migration.Run(context.Background(), path, urlConn, "down")
+		n, executed, err := migration.Run(cmd.Context(), path, urlConn, "down")
 		if err != nil {
 			return err
 		}
@@ -25,7 +24,7 @@ var resetCmd = &cobra.Command{
 		for _, e := range executed {
 			fmt.Fprintf(os.Stdout, "%v SUCCESS\n", e)
 		}
-		n, executed, err = migration.Run(context.Background(), path, urlConn, "up")
+		n, executed, err = migration.Run(cmd.Context(), path, urlConn, "up")
 		if err != nil {
 			return err
 		}
diff --git a/cmd/up.go b/cmd/up.go
index 91171b9..5aac3ad 100644
--- a/cmd/up.go
+++ b/cmd/up.go
@@ -1,7 +1,6 @@
 package cmd
 
 import (
-	"context"
 	"fmt"
 	"os"
 
@@ -16,7 +15,7 @@ var upCmd = &cobra.Command{
 	Long:    `Apply all available migrations`,
 	PreRunE: checkTable,
 	RunE: func(cmd *cobra.Command, args []string) error {
-		n, executed, err := migration.Run(context.Background(), path, urlConn, "up")
+		n, executed, err := migration.Run(cmd.Context(), path, urlConn, "up")
 		if err != nil {
 			return err
 		}
diff --git a/config/config.go b/config/config.go
index aef75c9..2d32798 100644
--- a/config/config.go
+++ b/config/config.go
@@ -13,6 +13,11 @@ import (
 	"github.com/structy/log"
 )
 
+const (
+	jsonAggDefault = "jsonb_agg"
+	jsonAgg        = "json_agg"
+)
+
 // TablesConf informations
 type TablesConf struct {
 	Name        string   `mapstructure:"name"`
@@ -92,6 +97,7 @@ type Prest struct {
 	JWTKey               string
 	JWTAlgo              string
 	JWTWhiteList         []string
+	JSONAggType          string
 	MigrationsPath       string
 	QueriesPath          string
 	AccessConf           AccessConf
@@ -119,6 +125,18 @@ var (
 	defaultCfgFile = "./prest.toml"
 )
 
+// Load configuration
+func Load() {
+	viperCfg()
+	PrestConf = &Prest{}
+	Parse(PrestConf)
+	if _, err := os.Stat(PrestConf.QueriesPath); os.IsNotExist(err) {
+		if err = os.MkdirAll(PrestConf.QueriesPath, 0700); os.IsNotExist(err) {
+			log.Errorf("Queries directory %s was not created\n", PrestConf.QueriesPath)
+		}
+	}
+}
+
 func viperCfg() {
 	configFile = getPrestConfFile(os.Getenv("PREST_CONF"))
 
@@ -161,6 +179,8 @@ func viperCfg() {
 	viper.SetDefault("jwt.algo", "HS256")
 	viper.SetDefault("jwt.whitelist", []string{"/auth"})
 
+	viper.SetDefault("json.agg.type", "jsonb_agg")
+
 	viper.SetDefault("cors.allowheaders", []string{"Content-Type"})
 	viper.SetDefault("cors.allowmethods", []string{"GET", "HEAD", "POST", "PUT", "DELETE", "OPTIONS"})
 	viper.SetDefault("cors.alloworigin", []string{"*"})
@@ -200,6 +220,7 @@ func getPrestConfFile(prestConf string) string {
 }
 
 // Parse pREST config
+// todo: split config onto methods to simplify this
 func Parse(cfg *Prest) {
 	err := viper.ReadInConfig()
 	if err != nil {
@@ -257,6 +278,9 @@ func Parse(cfg *Prest) {
 	cfg.JWTKey = viper.GetString("jwt.key")
 	cfg.JWTAlgo = viper.GetString("jwt.algo")
 	cfg.JWTWhiteList = viper.GetStringSlice("jwt.whitelist")
+
+	cfg.JSONAggType = getJSONAgg()
+
 	cfg.MigrationsPath = viper.GetString("migrations")
 	cfg.AccessConf.Restrict = viper.GetBool("access.restrict")
 	cfg.AccessConf.IgnoreTable = viper.GetStringSlice("access.ignore_table")
@@ -306,18 +330,6 @@ func Parse(cfg *Prest) {
 	cfg.PluginMiddlewareList = pluginMiddlewareConfig
 }
 
-// Load configuration
-func Load() {
-	viperCfg()
-	PrestConf = &Prest{}
-	Parse(PrestConf)
-	if _, err := os.Stat(PrestConf.QueriesPath); os.IsNotExist(err) {
-		if err = os.MkdirAll(PrestConf.QueriesPath, 0700); os.IsNotExist(err) {
-			log.Errorf("Queries directory %s was not created\n", PrestConf.QueriesPath)
-		}
-	}
-}
-
 // parseDatabaseURL tries to get from URL the DB configs
 func parseDatabaseURL(cfg *Prest) {
 	if cfg.PGURL == "" {
@@ -396,3 +408,18 @@ View more at https://docs.prestd.com/prestd/deployment/server-configuration`)
 	cfg.PGSSLCert = cfg.SSLCert
 	cfg.PGSSLRootCert = cfg.SSLRootCert
 }
+
+// getJSONAgg identifies which json aggregation function will be used,
+// support `jsonb` and `json`; `jsonb` is the default value
+//
+// https://www.postgresql.org/docs/9.5/functions-aggregate.html
+func getJSONAgg() (config string) {
+	config = viper.GetString("json.agg.type")
+	if config == jsonAgg {
+		return jsonAgg
+	}
+	if config != jsonAggDefault {
+		log.Warningln("JSON Agg type can only be 'json_agg' or 'jsonb_agg', using the later as default.")
+	}
+	return jsonAggDefault
+}
diff --git a/helpers/prest.go b/helpers/prest.go
index 5f5e0e6..e8a04fa 100644
--- a/helpers/prest.go
+++ b/helpers/prest.go
@@ -2,7 +2,7 @@ package helpers
 
 var (
 	// PrestVersionNumber repesemts prest version.
-	PrestVersionNumber = "1.1.6"
+	PrestVersionNumber = "1.4.0"
 	// CommitHash for version
 	CommitHash string
 )


Test patch:
diff --git a/config/config_test.go b/config/config_test.go
index 75a5812..064489b 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -93,6 +93,30 @@ func TestParse(t *testing.T) {
 		Parse(cfg)
 		require.Equal(t, "HS512", cfg.JWTAlgo)
 	})
+
+	t.Run("PREST_JSON_AGG_TYPE", func(t *testing.T) {
+		t.Setenv("PREST_JSON_AGG_TYPE", "invalid")
+		viperCfg()
+		cfg := &Prest{}
+		Parse(cfg)
+		require.Equal(t, jsonAggDefault, cfg.JSONAggType)
+	})
+
+	t.Run("PREST_JSON_AGG_TYPE backwards compatible", func(t *testing.T) {
+		t.Setenv("PREST_JSON_AGG_TYPE", jsonAgg)
+		viperCfg()
+		cfg := &Prest{}
+		Parse(cfg)
+		require.Equal(t, jsonAgg, cfg.JSONAggType)
+	})
+
+	t.Run("PREST_JSON_AGG_TYPE default works", func(t *testing.T) {
+		t.Setenv("PREST_JSON_AGG_TYPE", jsonAggDefault)
+		viperCfg()
+		cfg := &Prest{}
+		Parse(cfg)
+		require.Equal(t, jsonAggDefault, cfg.JSONAggType)
+	})
 }
 
 func Test_getPrestConfFile(t *testing.T) {