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

String should default to text field in Postgres #3459

Open
stanislavkozlovski opened this issue Aug 25, 2024 · 4 comments
Open

String should default to text field in Postgres #3459

stanislavkozlovski opened this issue Aug 25, 2024 · 4 comments

Comments

@stanislavkozlovski
Copy link

Expected behavior

  var name: String = ""

Should result in

  name                          text not null,

Actual behavior

  var name: String = ""

Should result in

  name                          varchar(255) not null,

Steps to reproduce

Just generate the code

Why?

It's PostgreSQL's official advice - https://wiki.postgresql.org/wiki/Don%27t_Do_This#Don.27t_use_varchar.28n.29_by_default

@rbygrave
Copy link
Member

Why

Following the JPA spec there to produce varchar(255) instead of text.

The question then becomes, should we not follow the JPA spec for Postgres and use text?

One implication if we did that is for projects that support multiple database platforms like Postgres and SQL Server. That application would now have different behaviour between when using Postgres and when using SQL Server for the case when text larger than 255 characters is persisted.

@stanislavkozlovski
Copy link
Author

I guess it's a harder sell if the JPA spec is such.

I personally would default to the suggested default so as to be most user friendly.

But if the goal of the ORM and JPA is to have as exact reproducible behavior between databases as possible - then it makes sense.

@rbygrave
Copy link
Member

Note that today we can change the default mapping per type and platform like:

https://ebean.io/apidoc/15/io.ebean.api/io/ebean/config/DatabaseConfig.html#addCustomMapping(io.ebean.config.dbplatform.DbType,java.lang.String,io.ebean.annotation.Platform)

Example

var database = Database.builder()
  .addCustomMapping(DbType.VARCHAR, "text", Platform.POSTGRES)
  .build();

@rbygrave
Copy link
Member

So pondering, if we made this change what would be the impact and how negative would it be?

For the case of an application against a single database, then it would be nice for Postgres to default to varchar (rather than varchar(255)). So that would be the positive motivation for making this change.

For the case of an application that runs against multiple different database types that include Postgres, then there is the potential for Postgres to use varchar and for another db to use varchar(255) when a @Column String property has no defined length. This isn't ideal per se as it introduces inconsistent behaviour between the 2 databases. The argument that this isn't a major issue is that ...

(A) The workaround is to explicitly specify the length, and this is actually the right thing to do regardless for this multiple database case.

(B) The default mapping of varchar(255) was never really a great idea in the first place and you'd hope that most people are explicitly specifying lengths for varchar as needed.

(C) This change would only apply to Postgres DDL going forward. It isn't changing or impacting any existing applications etc.

Hmmm.

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

No branches or pull requests

2 participants