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

Generated sql for ZonedDateTime is nvarchar(255) not datetime type #2314

Closed
almothafar opened this issue Aug 15, 2021 · 8 comments
Closed

Generated sql for ZonedDateTime is nvarchar(255) not datetime type #2314

almothafar opened this issue Aug 15, 2021 · 8 comments

Comments

@almothafar
Copy link

Expected behavior

Generated code should be datetimeoffset or at least datetime for SQL Server

Actual behavior

Currently, it generates nvarchar(255) for that column.

Steps to reproduce

@Entity
@Table(name = "SomeTable", schema = "dbo")
public class SomeModelData extends Model {
    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "ID")
    private Integer id;

    @Column(name = "Year")
    private Integer year;

    @Column(name = "Month")
    private Integer month;

    @Column(name = "Filename", nullable = false, length = 100)
    private String filename;

    @Column(name = "CreatedDate", nullable = false)
    private ZonedDateTime createdDate;

}

Generates:

create table [dbo].[SomeTable] (
  [ID]                          integer identity(1,1) not null,
  [Year]                        integer,
  [Month]                       integer,
  [Filename]                    nvarchar(100) not null,
  [CreatedDate]                 nvarchar(255) not null,
  constraint [pk_SomeTable] primary key ([ID])
);

Workaround

I just add columnDefinition = "datetimeoffset", to @Column

Just a side note: I really prefer to use a datatype that respects timezone for ZonedDateTime as datetimeoffset in SQL server, and just another note LocalDateTime makes it datetime2 for SQL server, which is fine I think.

@rbygrave
Copy link
Member

This does not reproduce, the generated ddl uses datetime2. What version of ebean are you using?

@almothafar
Copy link
Author

almothafar commented Aug 22, 2021

@rbygrave I got these versions:

val ebeanDependencies: Seq[sbt.ModuleID] =
  Seq(
    "io.ebean" % "ebean" % "12.11.1",
    "io.ebean" % "ebean-agent" % "12.11.1",
    "io.ebean" % "ebean-ddl-generator" % "12.11.1",
    "io.ebean" % "ebean-migration" % "12.11.0"
  )

image

[info]  - p.a.db.DefaultDBApi - Database [default] initialized
[info]  - p.a.d.HikariCPConnectionPool - Creating Pool for datasource 'default'
[info]  - c.z.h.HikariDataSource - HikariPool-1 - Starting...
[info]  - c.z.h.HikariDataSource - HikariPool-1 - Start completed.
[info]  - i.a.c.InitialLoadContext - loaded properties from [application.yaml]
[info]  - o.r.Reflections - Reflections took 85 ms to scan 2 urls, producing 82 keys and 3274 values 
[info]  - i.ebean.EbeanVersion - ebean version: 12.11.1
[info]  - i.e.s.d.p.AnnotationAssocOnes - Automatically determining join columns for @PrimaryKeyJoinColumn - ignoring PrimaryKeyJoinColumn.name attribute [Id] on ****
[info]  - i.e.s.d.p.AnnotationAssocOnes - Automatically determining join columns for @PrimaryKeyJoinColumn - Ignoring PrimaryKeyJoinColumn.referencedColumnName attribute [Id] on ****
[info]  - io.ebean.DB - started database[default] platform[SQLSERVER17] in 1838ms

@rbygrave
Copy link
Member

It does not reproduce.

Can you modify this PR to reproduce the issue? - ebean-orm-examples/example-minimal#7

The mapped datatype is datetime2

@almothafar
Copy link
Author

almothafar commented Sep 8, 2021

@rbygrave I got 2 play projects, I can see this issue happened in one project and not happening with another project, that's really strange (in the second generated as datetime2), I got the same configuration and everything.

But I got another issue that I'm debugging now, with the project that generates nvarchar(255) the date stored correctly with the timezone, with the project that generates datetime2 I got a very strange issue:

  • If I send the date as UTC the date got converted as CST (default system timezone) so it is -5 hours but the timezone stored as UTC
  • If I send the date as CST the date stays as is without any issue but the timezone saved as UTC

I really have no idea what is going on, still debugging it tho.

So do you have any issue storing dates with different time zone as if you see time zone like TimeZone.setDefault(TimeZone.getTimeZone("US/Central"));

P.S: In the debugging console the insert statement printed out is 100% working, and if I run it manually it is working correctly

@almothafar
Copy link
Author

almothafar commented Sep 8, 2021

OK, now I know why I got that part of the issue, I can reproduce my issue by inserting data as the following:

If I got entity like:

@Entity
@Table(name = "AssetStatus", schema = "dbo")
public class AssetStatusData extends Model {
    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "ID")
    private Long id;

    @Column(name = "StartDate", nullable = false, columnDefinition = "datetimeoffset")
    private ZonedDateTime startDate;

    @Column(name = "EndDate", columnDefinition = "datetimeoffset")
    private ZonedDateTime endDate;

}

And console printed correctly like:

[debug] - io.ebean.SQL - txn[] insert into [dbo].[AssetStatus] ([StartDate], [EndDate]) values (?,?) --bind(2021-09-08T08:38:54.772036400-05:00[US/Central],null)

So I tested 2 statements:

insert into [dbo].[AssetStatus] ([StartDate], [EndDate])
values (Cast('2021-09-08T08:38:54.772036400-05:00' as datetimeoffset), null)

And

insert into [dbo].[AssetStatus] ([StartDate], [EndDate])
values (Cast('2021-09-08T08:38:54.772036400-05:00' as datetime2), null)

The second statement reproduces that strange behavior, so it is clear that mapping ZonedDateTime to datettime2 is not really working fine.

Still, I'm not sure why I have that mapping in my first project doing it as nvarchar(255)

@almothafar
Copy link
Author

almothafar commented Sep 8, 2021

OK, finally I see what the issue, my bad I have a converter that I totally forgot about it:

@Converter(autoApply = true)
public class ZonedDateTimeConverter implements AttributeConverter<ZonedDateTime, String> {
    private static final String DATE_FORMAT = "yyyy-MM-dd HH:mm:ss.SSSSSSS xxx";

    @Override
    public String convertToDatabaseColumn(ZonedDateTime zonedDateTime) {
        return DateTimeFormatter.ofPattern(DATE_FORMAT).format(zonedDateTime);
    }

    @Override
    public ZonedDateTime convertToEntityAttribute(String dateAsString) {
        return ZonedDateTime.parse(dateAsString, DateTimeFormatter.ofPattern(DATE_FORMAT));
    }
}

So even with a converter, why it is affecting DDL?

Anyways, this converter solves my issue with incorrect data stored in the database.

@rbygrave
Copy link
Member

rbygrave commented Sep 9, 2021

So even with a converter, why it is affecting DDL?

Typically a AttributeConverter converts a non persistable type to a persistable type. There strictly isn't anything related to DDL in the JPA spec but with Ebean the view is that the database type of the AttributeConverter is the type that will be persisted and hence that is the type used for DDL - ebean needs to do this to support ddl generation and testing really.

As I see it, AttributeConverter is not "super great" in the sense that it is an abstraction on top of another abstraction - personally I view it more as one abstraction too many and I would instead consider taking control of that in application code but it depends on the use case - I don't recall ever using AttributeConverter myself though.

Anyways, this converter solves my issue with incorrect data stored in the database.

Cool. I'll close this issue then.

@rbygrave rbygrave closed this as completed Sep 9, 2021
@almothafar
Copy link
Author

@rbygrave I thought there are 2 issues to solve:

  • I think it better if we map ZonedDateTime to something that respects timezone.
  • Saving data should not be an issue as right now ZonedDateTime date storing data without timezone and it gets timezone from default database server timezone instead.

Is that should be in a different issue?

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

2 participants