xo / usql

Universal command-line interface for SQL databases
MIT License
8.88k stars 347 forks source link

Bug: postgres metadata reader; options are assigned twice #358

Closed Allam76 closed 2 years ago

Allam76 commented 2 years ago

Consider the following code:

func TestUsqlPostgresOptionsFail(t *testing.T) {
    db, _ := sql.Open("postgres", "postgres://martin:<pass>@localhost:5432/northwind")
    reader := postgres.NewReader()(db,infos.WithDataTypeFormatter(dataTypeFormatter)).(metadata.BasicReader)
    tableSet, err := reader.Tables(metadata.Filter{Schema: "public", Catalog: "northwind"})
    assert.NoError(t, err)
    assert.NotEmpty(t, tableSet)
}

Here I would like to add a custom data type formatter to the postgres metadata reader. This fails with: panic: interface conversion: metadata.Reader is *metadata.LoggingReader, not *informationschema.InformationSchema

From reading the offending code:

func NewReader() func(drivers.DB, ...metadata.ReaderOption) metadata.Reader {
    return func(db drivers.DB, opts ...metadata.ReaderOption) metadata.Reader {
        newIS := infos.New(
            infos.WithIndexes(false),
            infos.WithCustomClauses(map[infos.ClauseName]string{
                infos.ColumnsColumnSize:         "COALESCE(character_maximum_length, numeric_precision, datetime_precision, interval_precision, 0)",
                infos.FunctionColumnsColumnSize: "COALESCE(character_maximum_length, numeric_precision, datetime_precision, interval_precision, 0)",
            }),
            infos.WithSystemSchemas([]string{"pg_catalog", "pg_toast", "information_schema"}),
            infos.WithCurrentSchema("CURRENT_SCHEMA"),
            infos.WithDataTypeFormatter(dataTypeFormatter))
        return metadata.NewPluginReader(
            newIS(db, opts...),
            &metaReader{
                LoggingReader: metadata.NewLoggingReader(db, opts...),
            },
        )
    }
}

it is clear that the opts... are assigned twice. Last time to an incompatible reader (LoggingReader).

Maybe also consider putting

type metaReader struct

in uppercase to facilitate extension.

kenshaw commented 2 years ago

@Allam76 PRs are always welcome!

nineinchnick commented 2 years ago

The InformationSchema (IS) reader is independent from the postgres.metaReader. The fact that the postgres.metaReader uses parts of the IS reader is an implementation detail. We might remove that some day, and instead read the information directly from PostgreSQL's system catalogs, as we already do for some reader sub-interfaces. You can't pass the IS reader options to the postgres.metaReader.

The postgres.metaReader also is not meant to be extended. If you want to change its behavior, create a new reader. Feel free to copy the code from the postgres reader.

An alternative is to allow postgres.metaReader to receive a similar option to configure the data type formatter, but there would need to be a use case for it in usql itself. Right now, even if it's reused for multiple different databases compatible with PostgreSQL (like CockroachDB), it configures itself.

Why do you need to change the data type formatter in the first place?

nineinchnick commented 2 years ago

Or use the metadata.NewPluginReader() to override only the Columns() method in the postgres reader.