ronantakizawa/codereview-bench huggingface dataset:
Repository: hasura/graphql-engine
File: cli/migrate/migrate.go (Go)

Code before:

		ret <- migr
		go migr.Buffer()

		migr, err = m.newMigration(suint64(from), int64(prev))
		if err != nil {
			ret <- err
			return
		}

		ret <- migr
		go migr.Buffer()
		from = int64(prev)
		noOfAppliedMigrations++
	}
}

func (m *Migrate) ApplySeed(q interface{}) error {
	return m.databaseDrv.ApplySeed(q)
}

func (m *Migrate) ExportDataDump(tableNames []string) ([]byte, error) {
	// to support tables starting with capital letters
	modifiedTableNames := make([]string, len(tableNames))
	for idx, val := range tableNames {
		modifiedTableNames[idx] = "\"" + val + "\""
	}
	return m.databaseDrv.ExportDataDump(modifiedTableNames)
}

func printDryRunStatus(migrations []*Migration) *bytes.Buffer {
	out := new(tabwriter.Writer)
	buf := &bytes.Buffer{}
	out.Init(buf, 0, 8, 2, ' ', 0)
	w := util.NewPrefixWriter(out)
	w.Write(util.LEVEL_0, "VERSION\tTYPE\tNAME\n")
	for _, migration := range migrations {
		var direction string
		if int64(migration.Version) == migration.TargetVersion {
			direction = "up"
		} else {
			direction = "down"
		}
		w.Write(util.LEVEL_0, "%d\t%s\t%s\n",
			migration.Version,
			direction,
			migration.Identifier,
		)
	}
	out.Flush()
	return buf

Review comment (suggestion):
May be a bit pedantic, but can we change this to ?
```suggestion
		modifiedTableNames[idx] = fmt.Sprintf(`"%s"`, val) 
```

Code after:

		ret <- migr
		go migr.Buffer()

		migr, err = m.newMigration(suint64(from), int64(prev))
		if err != nil {
			ret <- err
			return
		}

		ret <- migr
		go migr.Buffer()
		from = int64(prev)
		noOfAppliedMigrations++
	}
}

func (m *Migrate) ApplySeed(q interface{}) error {
	return m.databaseDrv.ApplySeed(q)
}

func (m *Migrate) ExportDataDump(tableNames []string) ([]byte, error) {
	// to support tables starting with capital letters
	modifiedTableNames := make([]string, len(tableNames))
	for idx, val := range tableNames {
		modifiedTableNames[idx] = fmt.Sprintf(`"%s"`, val) 
	}
	return m.databaseDrv.ExportDataDump(modifiedTableNames)
}

func printDryRunStatus(migrations []*Migration) *bytes.Buffer {
	out := new(tabwriter.Writer)
	buf := &bytes.Buffer{}
	out.Init(buf, 0, 8, 2, ' ', 0)
	w := util.NewPrefixWriter(out)
	w.Write(util.LEVEL_0, "VERSION\tTYPE\tNAME\n")
	for _, migration := range migrations {
		var direction string
		if int64(migration.Version) == migration.TargetVersion {
			direction = "up"
		} else {
			direction = "down"
		}
		w.Write(util.LEVEL_0, "%d\t%s\t%s\n",
			migration.Version,
			direction,
			migration.Identifier,
		)
	}
	out.Flush()
	return buf
Repository: hasura/graphql-engine
File: console/src/components/Services/Data/TableModify/ModifyActions.js (JavaScript)

Code before:
          newCustomColumnNames
        )
      );
      schemaChangesDown.push(
        getSetCustomRootFieldsQuery(
          tableDef,
          existingRootFields,
          existingCustomColumnNames
        )
      );
    }

    const colDefaultWithQuotes =
      isColTypeString(colType) && !isPostgresFunction(colDefault)
        ? `'${colDefault}'`
        : colDefault;
    const originalColDefaultWithQuotes =
      isColTypeString(colType) && !isPostgresFunction(originalColDefault)
        ? `'${originalColDefault}'`
        : originalColDefault;

    /* column default up/down migration */
    let columnDefaultUpQuery;
    let columnDefaultDownQuery;
    if (colDefault !== '') {
      // ALTER TABLE ONLY <table> ALTER COLUMN <column> SET DEFAULT <default>;
      columnDefaultUpQuery =
        'ALTER TABLE ' +
        '"' +
        currentSchema +
        '"' +
        '.' +
        '"' +
        tableName +
        '"' +
        ' ALTER COLUMN ' +
        '"' +
        colName +
        '"' +
        ' SET DEFAULT ' +
        colDefaultWithQuotes +
        ';';
    } else {
      // ALTER TABLE <table> ALTER COLUMN <column> DROP DEFAULT;
      columnDefaultUpQuery =
        'ALTER TABLE ' +
        '"' +
        currentSchema +
        '"' +
        '.' +
        '"' +

Review comment (suggestion):
```suggestion
      // ALTER TABLE <table> ALTER COLUMN <column> SET DEFAULT <default>;
```

Code after:
          newCustomColumnNames
        )
      );
      schemaChangesDown.push(
        getSetCustomRootFieldsQuery(
          tableDef,
          existingRootFields,
          existingCustomColumnNames
        )
      );
    }

    const colDefaultWithQuotes =
      isColTypeString(colType) && !isPostgresFunction(colDefault)
        ? `'${colDefault}'`
        : colDefault;
    const originalColDefaultWithQuotes =
      isColTypeString(colType) && !isPostgresFunction(originalColDefault)
        ? `'${originalColDefault}'`
        : originalColDefault;

    /* column default up/down migration */
    let columnDefaultUpQuery;
    let columnDefaultDownQuery;
    if (colDefault !== '') {
      // ALTER TABLE <table> ALTER COLUMN <column> SET DEFAULT <default>;
      columnDefaultUpQuery =
        'ALTER TABLE ' +
        '"' +
        currentSchema +
        '"' +
        '.' +
        '"' +
        tableName +
        '"' +
        ' ALTER COLUMN ' +
        '"' +
        colName +
        '"' +
        ' SET DEFAULT ' +
        colDefaultWithQuotes +
        ';';
    } else {
      // ALTER TABLE <table> ALTER COLUMN <column> DROP DEFAULT;
      columnDefaultUpQuery =
        'ALTER TABLE ' +
        '"' +
        currentSchema +
        '"' +
        '.' +
        '"' +
Repository: hasura/graphql-engine
File: cli/migrate/migrate.go (Go)

Code before:
		ret <- migr
		go migr.Buffer()

		migr, err = m.newMigration(suint64(from), int64(prev))
		if err != nil {
			ret <- err
			return
		}

		ret <- migr
		go migr.Buffer()
		from = int64(prev)
		noOfAppliedMigrations++
	}
}

func (m *Migrate) ApplySeed(q interface{}) error {
	return m.databaseDrv.ApplySeed(q)
}

func (m *Migrate) ExportDataDump(tableNames []string) ([]byte, error) {
	// to support tables starting with capital letters
	modifiedTableNames := make([]string, len(tableNames))
	for idx, val := range tableNames {
		split := strings.Split(val, ".")
		if len(split) == 2 {
			modifiedTableNames[idx] = fmt.Sprintf(`%s."%s"`, split[0], split[1])
		} else {
			modifiedTableNames[idx] = fmt.Sprintf(`"%s"`, val)
		}
	}
	return m.databaseDrv.ExportDataDump(modifiedTableNames)
}

func printDryRunStatus(migrations []*Migration) *bytes.Buffer {
	out := new(tabwriter.Writer)
	buf := &bytes.Buffer{}
	out.Init(buf, 0, 8, 2, ' ', 0)
	w := util.NewPrefixWriter(out)
	w.Write(util.LEVEL_0, "VERSION\tTYPE\tNAME\n")
	for _, migration := range migrations {
		var direction string
		if int64(migration.Version) == migration.TargetVersion {
			direction = "up"
		} else {
			direction = "down"
		}
		w.Write(util.LEVEL_0, "%d\t%s\t%s\n",
			migration.Version,
			direction,
			migration.Identifier,

Review comment (suggestion):
I think we should be throwing an error if `len(split)` isn't either 2 or 1, as it's not a valid table/schema name.

Code after:
		ret <- migr
		go migr.Buffer()

		migr, err = m.newMigration(suint64(from), int64(prev))
		if err != nil {
			ret <- err
			return
		}

		ret <- migr
		go migr.Buffer()
		from = int64(prev)
		noOfAppliedMigrations++
	}
}

func (m *Migrate) ApplySeed(q interface{}) error {
	return m.databaseDrv.ApplySeed(q)
}

func (m *Migrate) ExportDataDump(tableNames []string) ([]byte, error) {
	// to support tables starting with capital letters
	modifiedTableNames := make([]string, len(tableNames))
	
	for idx, val := range tableNames {
		split := strings.Split(val, ".")
		splitLen := len(split)
		
		if splitLen != 1 && splitLen != 2 {
			return nil, fmt.Errorf(`invalid schema/table provided "%s"`, val)
		}

		if splitLen == 2 {
			modifiedTableNames[idx] = fmt.Sprintf(`"%s"."%s"`, split[0], split[1])
		} else {
			modifiedTableNames[idx] = fmt.Sprintf(`"%s"`, val)
		}
	}
	
	return m.databaseDrv.ExportDataDump(modifiedTableNames)
}

func printDryRunStatus(migrations []*Migration) *bytes.Buffer {
	out := new(tabwriter.Writer)
	buf := &bytes.Buffer{}
	out.Init(buf, 0, 8, 2, ' ', 0)
	w := util.NewPrefixWriter(out)
	w.Write(util.LEVEL_0, "VERSION\tTYPE\tNAME\n")
	for _, migration := range migrations {
		var direction string
		if int64(migration.Version) == migration.TargetVersion {
Repository: hasura/graphql-engine
File: server/src-lib/Hasura/RQL/Types/Common.hs (Haskell)

Code before:
       , Constraint(..)
       , PrimaryKey(..)
       , pkConstraint
       , pkColumns
       , ForeignKey(..)
       , EquatableGType(..)
       , InpValInfo(..)
       , CustomColumnNames

       , NonEmptyText
       , mkNonEmptyTextUnsafe
       , mkNonEmptyText
       , unNonEmptyText
       , nonEmptyText
       , adminText
       , rootText

       , SystemDefined(..)
       , isSystemDefined

       , successMsg
       , NonNegativeDiffTime(..)
       , InputWebhook(..)
       , ResolvedWebhook(..)
       , resolveWebhook
       , NonNegativeInt(..)
       ) where

import           Hasura.EncJSON
import           Hasura.Incremental            (Cacheable)
import           Hasura.Prelude
import           Hasura.RQL.DDL.Headers        ()
import           Hasura.RQL.Types.Error
import           Hasura.SQL.Types



import           Control.Lens                  (makeLenses)
import           Data.Aeson
import           Data.Aeson.Casing
import           Data.Aeson.TH
import           Data.URL.Template
import           Instances.TH.Lift             ()
import           Language.Haskell.TH.Syntax    (Lift, Q, TExp)

import qualified Data.Environment              as Env
import qualified Data.HashMap.Strict           as HM
import           Data.Scientific               (toBoundedInteger)
import qualified Data.Text                     as T
import qualified Database.PG.Query             as Q
import qualified Language.GraphQL.Draft.Syntax as G

Review comment (suggestion):
```suggestion
       , NonNegativeInt
```
Exposing the constructor of `NonNegativeInt` may not be a very good idea. Instead, you can expose the `mkNonNegativeInt` function that I have suggested.

Code after:
       , Constraint(..)
       , PrimaryKey(..)
       , pkConstraint
       , pkColumns
       , ForeignKey(..)
       , EquatableGType(..)
       , InpValInfo(..)
       , CustomColumnNames

       , NonEmptyText
       , mkNonEmptyTextUnsafe
       , mkNonEmptyText
       , unNonEmptyText
       , nonEmptyText
       , adminText
       , rootText

       , SystemDefined(..)
       , isSystemDefined

       , successMsg
       , NonNegativeDiffTime
       , unNonNegativeDiffTime
       , unsafeNonNegativeDiffTime
       , mkNonNegativeDiffTime
       , InputWebhook(..)
       , ResolvedWebhook(..)
       , resolveWebhook
       , NonNegativeInt
       , getNonNegativeInt
       , mkNonNegativeInt
       , unsafeNonNegativeInt
       , Timeout(..)
       , defaultActionTimeoutSecs
       ) where

import           Hasura.EncJSON
import           Hasura.Incremental            (Cacheable)
import           Hasura.Prelude
import           Hasura.RQL.DDL.Headers        ()
import           Hasura.RQL.Types.Error
import           Hasura.SQL.Types



import           Control.Lens                  (makeLenses)
import           Data.Aeson
import           Data.Aeson.Casing
import           Data.Bifunctor                (bimap)
import           Data.Aeson.TH
import           Data.Scientific               (toBoundedInteger)
Repository: hasura/graphql-engine
File: server/src-lib/Hasura/Server/Init/Config.hs (Haskell)

Code before:

instance FromEnv AdminSecretHash where
  fromEnv = Right . hashAdminSecret . T.pack

instance FromEnv RoleName where
  fromEnv string = case mkRoleName (T.pack string) of
    Nothing       -> Left "empty string not allowed"
    Just roleName -> Right roleName

instance FromEnv Bool where
  fromEnv = parseStrAsBool

instance FromEnv Q.TxIsolation where
  fromEnv = readIsoLevel

instance FromEnv CorsConfig where
  fromEnv = readCorsDomains

instance FromEnv [API] where
  fromEnv = readAPIs

instance FromEnv LQ.BatchSize where
  fromEnv = fmap LQ.BatchSize . readEither

instance FromEnv LQ.RefetchInterval where
  fromEnv = fmap (LQ.RefetchInterval . NonNegativeDiffTime . milliseconds . fromInteger) . readEither

instance FromEnv Milliseconds where
  fromEnv = fmap fromInteger . readEither

instance FromEnv JWTConfig where
  fromEnv = readJson

instance L.EnabledLogTypes impl => FromEnv [L.EngineLogType impl] where
  fromEnv = L.parseEnabledLogTypes

instance FromEnv L.LogLevel where
  fromEnv = readLogLevel

instance FromEnv Cache.CacheSize where
  fromEnv = Cache.parseCacheSize

type WithEnv a = ReaderT Env (ExceptT String Identity) a

runWithEnv :: Env -> WithEnv a -> Either String a
runWithEnv env m = runIdentity $ runExceptT $ runReaderT m env


Review comment (suggestion):
Instead of using the `NonNegativeDiffTime` constructor here, use the `mkNonNegativeDiffTime`. 

In the current implementation, negative values will also be accepted. To avoid it, we need to use the `mkNonNegativeDiffTime`


Code after:
instance FromEnv AdminSecretHash where
  fromEnv = Right . hashAdminSecret . T.pack

instance FromEnv RoleName where
  fromEnv string = case mkRoleName (T.pack string) of
    Nothing       -> Left "empty string not allowed"
    Just roleName -> Right roleName

instance FromEnv Bool where
  fromEnv = parseStrAsBool

instance FromEnv Q.TxIsolation where
  fromEnv = readIsoLevel

instance FromEnv CorsConfig where
  fromEnv = readCorsDomains

instance FromEnv [API] where
  fromEnv = readAPIs

instance FromEnv LQ.BatchSize where
  fromEnv s = do
    val <- readEither s
    maybe (Left "batch size should be a non negative integer") Right $ LQ.mkBatchSize val

instance FromEnv LQ.RefetchInterval where
  fromEnv x = do
    val <- fmap (milliseconds . fromInteger) . readEither $ x
    maybe (Left "refetch interval should be a non negative integer") Right $ LQ.mkRefetchInterval val

instance FromEnv Milliseconds where
  fromEnv = fmap fromInteger . readEither

instance FromEnv JWTConfig where
  fromEnv = readJson

instance L.EnabledLogTypes impl => FromEnv [L.EngineLogType impl] where
  fromEnv = L.parseEnabledLogTypes

instance FromEnv L.LogLevel where
  fromEnv = readLogLevel

instance FromEnv Cache.CacheSize where
  fromEnv = Cache.parseCacheSize

type WithEnv a = ReaderT Env (ExceptT String Identity) a

runWithEnv :: Env -> WithEnv a -> Either String a
runWithEnv env m = runIdentity $ runExceptT $ runReaderT m env
Repository: hasura/graphql-engine
File: server/src-lib/Hasura/GraphQL/Execute/LiveQuery/Options.hs (Haskell)

Code before:
  } deriving (Show, Eq)

mkLiveQueriesOptions :: Maybe BatchSize -> Maybe RefetchInterval -> LiveQueriesOptions
mkLiveQueriesOptions batchSize refetchInterval = LiveQueriesOptions
  { _lqoBatchSize = fromMaybe (BatchSize 100) batchSize
  , _lqoRefetchInterval = fromMaybe (RefetchInterval 1) refetchInterval
  }

instance J.ToJSON LiveQueriesOptions where
  toJSON (LiveQueriesOptions batchSize refetchInterval) =
    J.object [ "batch_size" J..= batchSize
             , "refetch_delay" J..= refetchInterval
             ]

instance J.FromJSON LiveQueriesOptions where
  parseJSON = J.withObject "live query options" \o ->
    LiveQueriesOptions <$> o J..: "batch_size"
                       <*> o J..: "refetch_delay"

newtype BatchSize = BatchSize { unBatchSize :: NonNegativeInt }
  deriving (Show, Eq, J.ToJSON, J.FromJSON)

mkBatchSize :: Int -> Maybe BatchSize
mkBatchSize x = do
  t <- mkNonNegativeInt x
  return $ BatchSize t 

-- TODO this is treated as milliseconds in fromEnv and as seconds in ToJSON.
--      ideally this would have e.g. ... unRefetchInterval :: Milliseconds
newtype RefetchInterval = RefetchInterval { unRefetchInterval :: NonNegativeDiffTime }
  deriving (Show, Eq, J.ToJSON, J.FromJSON)

mkRefetchInterval :: DiffTime -> Maybe RefetchInterval
mkRefetchInterval x = do
  t <- mkNonNegativeDiffTime x
  return $ RefetchInterval t


Review comment (performance):
```suggestion
mkBatchSize :: Int -> Maybe BatchSize
mkBatchSize =  BatchSize <$> mkNonNegativeInt
```

Code after:
  { _lqoBatchSize       :: !BatchSize
  , _lqoRefetchInterval :: !RefetchInterval
  } deriving (Show, Eq)

mkLiveQueriesOptions :: Maybe BatchSize -> Maybe RefetchInterval -> LiveQueriesOptions
mkLiveQueriesOptions batchSize refetchInterval = LiveQueriesOptions
  { _lqoBatchSize = fromMaybe (BatchSize 100) batchSize
  , _lqoRefetchInterval = fromMaybe (RefetchInterval 1) refetchInterval
  }

instance J.ToJSON LiveQueriesOptions where
  toJSON (LiveQueriesOptions batchSize refetchInterval) =
    J.object [ "batch_size" J..= batchSize
             , "refetch_delay" J..= refetchInterval
             ]

instance J.FromJSON LiveQueriesOptions where
  parseJSON = J.withObject "live query options" \o ->
    LiveQueriesOptions <$> o J..: "batch_size"
                       <*> o J..: "refetch_delay"

newtype BatchSize = BatchSize { unBatchSize :: NonNegativeInt }
  deriving (Show, Eq, J.ToJSON, J.FromJSON)

mkBatchSize :: Int -> Maybe BatchSize
mkBatchSize x = BatchSize <$> mkNonNegativeInt x

-- TODO this is treated as milliseconds in fromEnv and as seconds in ToJSON.
--      ideally this would have e.g. ... unRefetchInterval :: Milliseconds
newtype RefetchInterval = RefetchInterval { unRefetchInterval :: NonNegativeDiffTime }
  deriving (Show, Eq, J.ToJSON, J.FromJSON)

mkRefetchInterval :: DiffTime -> Maybe RefetchInterval
mkRefetchInterval x = RefetchInterval <$> mkNonNegativeDiffTime x
Repository: hasura/graphql-engine
File: server/src-test/Data/NonNegativeIntSpec.hs (Haskell)

Code before:
module Data.NonNegativeIntSpec (spec) where
-- | basic tests on NonNegativeIntType

import           Hasura.RQL.Types.Common (NonNegativeInt, mkNonNegativeInt)
import           Prelude
import           Test.Hspec              (Spec, describe, it, shouldBe)

evalMaybeNonNegativeInt :: Maybe NonNegativeInt -> Bool
evalMaybeNonNegativeInt x = case x of
    Just _  -> True
    Nothing -> False

spec :: Spec
spec = do
  nonNegIntSpec

nonNegIntSpec :: Spec
nonNegIntSpec =
    describe "non negative integer type" $ do
        it "only validates non negative integers" $ do
            (evalMaybeNonNegativeInt $ mkNonNegativeInt 23) `shouldBe` True
            (evalMaybeNonNegativeInt $ mkNonNegativeInt (-23)) `shouldBe` False



Review comment (suggestion):
You can directly compare the value instead of converting it into a `Bool` and I think it will be more useful that way.
You can do `mkNonNegativeInt shouldBe (Just 23)` 

Code after:
module Data.NonNegativeIntSpec (spec) where
-- | basic tests on NonNegativeIntType

import           Prelude

import           Hasura.RQL.Types.Common (mkNonNegativeInt)

import           Test.Hspec              (Spec, describe, it, shouldBe)

spec :: Spec
spec = do
  nonNegIntSpec

nonNegIntSpec :: Spec
nonNegIntSpec =
    describe "non negative integer type" $ do
        it "only validates non negative integers" $ do
            (mkNonNegativeInt 23) `shouldBe` (Just 23)
            (mkNonNegativeInt (-23)) `shouldBe` Nothing

 -- TODO: add spec for fromJSON for NonNegativeInt type
Repository: hasura/graphql-engine
File: server/src-lib/Hasura/RQL/Types/Common.hs (Haskell)

Code before:

newtype ResolvedWebhook
  = ResolvedWebhook { unResolvedWebhook :: Text}
  deriving ( Show, Eq, FromJSON, ToJSON, Hashable, DQuote, Lift)

newtype InputWebhook
  = InputWebhook {unInputWebhook :: URLTemplate}
  deriving (Show, Eq, Lift, Generic)
instance NFData InputWebhook
instance Cacheable InputWebhook

instance ToJSON InputWebhook where
  toJSON =  String . printURLTemplate . unInputWebhook

instance FromJSON InputWebhook where
  parseJSON = withText "String" $ \t ->
    case parseURLTemplate t of
      Left e  -> fail $ "Parsing URL template failed: " ++ e
      Right v -> pure $ InputWebhook v

instance Q.FromCol InputWebhook where
  fromCol bs = do
    urlTemplate <- parseURLTemplate <$> Q.fromCol bs
    case urlTemplate of
      Left e -> Left $ "Parsing URL template failed: " <> T.pack e
      Right v -> Right $ InputWebhook v

resolveWebhook :: QErrM m => Env.Environment -> InputWebhook -> m ResolvedWebhook
resolveWebhook env (InputWebhook urlTemplate) = do
  let eitherRenderedTemplate = renderURLTemplate env urlTemplate
  either (throw400 Unexpected . T.pack)
    (pure . ResolvedWebhook) eitherRenderedTemplate


Review comment (bug):
```suggestion
    bimap (\e-> "Parsing URL template failed: " <> T.pack e) InputWebhook urlTemplate
```
only if you want. You'll need to import `Data.Bifunctor` probably.

Code after:
      False -> fail "negative value not allowed"

newtype ResolvedWebhook
  = ResolvedWebhook { unResolvedWebhook :: Text}
  deriving ( Show, Eq, FromJSON, ToJSON, Hashable, DQuote, Lift)

newtype InputWebhook
  = InputWebhook {unInputWebhook :: URLTemplate}
  deriving (Show, Eq, Lift, Generic)
instance NFData InputWebhook
instance Cacheable InputWebhook

instance ToJSON InputWebhook where
  toJSON =  String . printURLTemplate . unInputWebhook

instance FromJSON InputWebhook where
  parseJSON = withText "String" $ \t ->
    case parseURLTemplate t of
      Left e  -> fail $ "Parsing URL template failed: " ++ e
      Right v -> pure $ InputWebhook v

instance Q.FromCol InputWebhook where
  fromCol bs = do
    urlTemplate <- parseURLTemplate <$> Q.fromCol bs
    bimap (\e -> "Parsing URL template failed: " <> T.pack e) InputWebhook urlTemplate

resolveWebhook :: QErrM m => Env.Environment -> InputWebhook -> m ResolvedWebhook
resolveWebhook env (InputWebhook urlTemplate) = do
  let eitherRenderedTemplate = renderURLTemplate env urlTemplate
  either (throw400 Unexpected . T.pack)
    (pure . ResolvedWebhook) eitherRenderedTemplate