Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom Column Type #273

Open
amanbolat opened this issue Sep 16, 2023 · 11 comments
Open

Custom Column Type #273

amanbolat opened this issue Sep 16, 2023 · 11 comments

Comments

@amanbolat
Copy link

amanbolat commented Sep 16, 2023

Is your feature request related to a problem? Please describe.

Currently, go-jet generator supports only a limited type of columns. For example, for uuid, text or bytea columns only the postgres.ColumnString type is used.

The issues I'm facing at the moment is I cannot derive the real type of the column, and therefore it's impossible to dynamically build WHERE clause.

Example SQL table:

CREATE TABLE table1 (
    id              UUID PRIMARY KEY,
    value           TEXT
);

Now, I have a function that creates postgres.BoolExpression based on the column type:

func (b JetFilterBuilder) buildEqExpression(col postgres.Column, value string) (postgres.BoolExpression, error) {
	switch typedCol := col.(type) {
	case postgres.ColumnString:
		return typedCol.EQ(postgres.String(value)), nil
        // Skip rest of the code.
}

If I use table.Table1.Id column and pass some text value, it will create SQL like this one:

WHERE id = 'value'::text

Describe the solution you'd like

Options

Option 1

Enable the user to use custom column type during the generation. Right now, I think it's not possible using the generator.
Example of my attempt:

return tableBuilder.UseColumn(func(column metadata.Column) template.TableSQLBuilderColumn {
	if column.DataType.Name == "uuid" {
		return template.TableSQLBuilderColumn{
			Name: column.Name,
			Type: "UUID",
		}
	}

	return template.DefaultTableSQLBuilderColumn(column)
})

Will generate:

type table1 struct {
	postgres.Table

	//Columns
	id               postgres.ColumnUUID // <---------- type doesn't exists.
|

Would be nice to have the same ability to pass a type, as it's done for model field type.

Option 2

Add real column type and other metadata to the Column instance. However, it might increase the size of the column object.

My current solution

I run a tool that will parse the AST tree of the generated file and change the column type to the custom one.

Custom column type example:

type ColumnUUID struct {
	postgres.ColumnString
}

func UUIDColumn(val string) ColumnUUID {
	return ColumnUUID{postgres.StringColumn(val)}
}

Final goal

Assume I had the ability to introduce my custom column, my code would look like:

func (b JetFilterBuilder) buildEqExpression(col postgres.Column, value string) (postgres.BoolExpression, error) {
	switch typedCol := col.(type) {
	case postgres.ColumnString:
		return typedCol.EQ(postgres.String(value)), nil
	case custom_package.ColumnUUID:
                uuidVal := parseUUID(value)
		return typedCol.EQ(postgres.UUID(uuidVal)), nil
        // Skip rest of the code.
}

Thoughts

Maybe I missed something, and it's still possible to achieve my goal without any changes to the current version of go-jet, yet I haven't found one.

When I faced the issue, it seemed to me that there is no value in not exposing jet internal interfaces and structs, that could make other developer's life much easier, by extending the library with custom types and other features.

@go-jet
Copy link
Owner

go-jet commented Sep 18, 2023

Hi @amanbolat,
As I see it, the issue is that postgres.String adds a mandatory ::text cast. And that's unfortunate because behind postgres.String, there can be any of the currently unsupported types (like json, uuid, etc...). The simplest solution would be to have a string literal function without sql cast. Luckily, there is one already postgres.UUID, so a workaround in your case would be:

func (b JetFilterBuilder) buildEqExpression(col postgres.Column, value string) (postgres.BoolExpression, error) {
	switch typedCol := col.(type) {
	case postgres.ColumnString:
		return typedCol.EQ(postgres.UUID(value)), nil   //<-- Use UUID constructor for both text and uuid columns
        // Skip rest of the code.
}

I like both of your ideas, but I think once we add support for missing types, there would be much need for such a feature.

@amanbolat
Copy link
Author

Hi @go-jet,

Thank you for the suggestion, yet I need an explicit check in case of UUID as in my example:

case custom_package.ColumnUUID:
    uuidVal := parseUUID(value)
    return typedCol.EQ(postgres.UUID(uuidVal)), nil

The same will be required for JSON and JSONB types for my use case.

My current solution of extending the generator with AST parsing and substitution works as expected, but it feels as a hack.

Nevertheless, I suppose it's hard to keep the library up to date with all the possible types and different databases. Therefore, exposing internal types and interfaces seems a better solution in a long term.

I would like to make some contribution and open the PRs, however I'm still trying to understand all the internal architecture to make sure that my requirements do align with your vision.

@go-jet
Copy link
Owner

go-jet commented Sep 18, 2023

Aha, I see. In that case we can go with option 1. It should be a small change. We can add a new field into TableSQLBuilderColumn something like TypeTemplate jet.ColumnExpression . If TypeTemplate is nil, Type field is used.
As other code of interest take a look at process.go processTableSQLBuilder and file_templates.go tableSQLBuilderTemplate.

@dragondgold
Copy link

Are there any plans to support custom types?

@houten11
Copy link

This issue is more about adding additional type information to the SQL builder column types.
You can customize the generator to use your custom model type, and use raw expressions if needed for sql builder part.

@dragondgold
Copy link

dragondgold commented Oct 14, 2024

This issue is more about adding additional type information to the SQL builder column types.
You can customize the generator to use your custom model type, and use raw expressions if needed for sql builder part.

Yes, sorry, I meant additional type information for the SQL Builder because I'm having a similar issue. In my case my primary keys and some indexed columns in Postgres are varchar(41) and as they are strings jet is type casting them with ::text which prevents postgres from using the indexes. I workaround this by using postgres.UUID as you mentioned but you need to remember to use that instead of String every time, I was looking for a way to enforce that

@houten11
Copy link

Would defining a new VARCHAR41 constructor work for you?

func VARCHAR41(str string) StringExpression {
	return StringExp(CAST(String(str)).AS("varchar(41)"))
}

@dragondgold
Copy link

Would defining a new VARCHAR41 constructor work for you?

func VARCHAR41(str string) StringExpression {
	return StringExp(CAST(String(str)).AS("varchar(41)"))
}

Yes! That's kind of what I have right now. The issue is that nothing prevents me from not using VARCHAR41() and just use String(). I was looking for a way to force that with types.

@go-jet
Copy link
Owner

go-jet commented Oct 23, 2024

String type is to general. I believe a solution for all the issues in these thread is to implement separate expression types for TEXT, VARCHAR, UUID, etc ... This way we will ensure type safety and enough information to build dynamic queries based on column type.

@dragondgold
Copy link

String type is to general. I believe a solution for all the issues in these thread is to implement separate expression types for TEXT, VARCHAR, UUID, etc ... This way we will ensure type safety and enough information to build dynamic queries based on column type.

Agreed!! That would be really nice

@go-jet
Copy link
Owner

go-jet commented Nov 3, 2024

For now, in release v2.12.0, added new constructors for Postgres-specific character types, including Text, Char, and VarChar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants