Issue Details (XML | Word | Printable)

Key: NUCJPA-66
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Testcase Required Testcase Required
Assignee: Unassigned
Reporter: Todd Vierling
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
DataNucleus API JPA

@Column(columnDefinition) implementation is wrong

Created: 12/Feb/10 01:01 AM   Updated: 26/Apr/10 03:40 PM   Resolved: 14/Apr/10 01:47 PM
Component/s: API
Affects Version/s: 2.0.1
Fix Version/s: 2.1.0.m2

Environment: Should affect all RDBMS's; tested on PostgreSQL.

Datastore: PostgreSQL
Severity: Development


 Description  « Hide
According to NUCJPA-28, @Column(columnDefinition) is now implemented as a string appended to the inferred type when generating schema. This is wrong. The whole point is to allow the programmer to specify the column type exactly, e.g.:

  @Basic
  @Column(columnDefinition = "numeric(5,2) default 1.01")
  float variance;

Here, rather than using the driver-specific mapping for 'float', the programmer is telling you to use exactly the SQL standard 'numeric' type with explicit precision and scale, DDL default value, and no 'not null' constraint.

If columnDefinition is set, it should override all the related attributes (e.g., nullable, length, precision, scale), as the columnDefinition contains the EXACT backend-specific DDL definition for that column's type including all of these four attributes. The Javadoc for @Column is sparse, but it specifically notes:


public abstract String columnDefinition
    (Optional) The SQL fragment that is used when generating the DDL for the column.
    Defaults to the generated SQL to create a column of the inferred type.


Taking the contrapositive of this, if columnDefinition is *not* the default (empty) value, then the inferred type is not used at all. Yes, this means that a programmer can make a type that conflicts with the Java type. However, that flexibility is the whole point: how else can you make the schema generator spit out the type "vendorspecific_floatingpoint2" (which otherwise behaves like a float or double in JDBC)?


Todd Vierling added a comment - 12/Feb/10 01:06 AM
Oh, and related, if I try to create a type thus:

  @Basic
  @Column(precision = 5, scale = 2)
  float variance;

I don't get a numeric(5,2) column - I still get the driver's equivalent datatype for 'float' (which, amusingly, is 'double precision' on PostgreSQL, even though 'real' is the direct counterpart). This is not technically a bug as it is not JPA-specified to work (AFAICT), but it should illustrate the problem with the improper columnDefinition logic.

A numeric(5,2) column doesn't need a BigDecimal to do its job; a float can handle those values fine, and JDBC is happy to handle the value that way too. As of this writing, there's no way to map a non-BigDecimal type to a numeric(p,s) DDL in the schema generator.

Andy Jefferson made changes - 18/Feb/10 12:45 PM
Field Original Value New Value
Priority Major [ 3 ] Incomplete [ 6 ]
Andy Jefferson made changes - 20/Feb/10 07:37 AM
Comment [ Point of correction: The "final time" link to the zipfile-based testcase format in the comment above was the very first time I was made aware of the desire for a specialized testcase format. That info is certainly not elsewhere in this report, nor the one to which I linked (which I found thanks to Google, not docs).

I didn't know there was a specific format until well after the knee-jerk initial response of "Testcase, as per the docs etc etc ? Patch ?". No link in that comment, so I supplied a testcase in an otherwise generally accepted full-class format. If more were desired, a note more along the lines of "Could you please create a testcase in the format described at <foo>?" would have come off as far more friendly.

Please keep this in mind when interacting with other people, who quite possibly could be knowledgeable software engineers. Politeness goes a long way. ]
Andy Jefferson made changes - 20/Feb/10 07:37 AM
Comment [ Seriously, this is how you deal with possible contributors?

I don't *need* a fix. I can simply use EclipseLink or OpenJPA, both of which get it right. I could have left it waiting for someone else to run into a brick wall.

But rather than let it linger, I signed up for a JIRA account for the first time in order to report this bug to *help you*. It doesn't matter to me if the bug report is in some mouse-clickable zipfile format that I haven't heard of until now. Neither should you, if the problem is well-described.

These responses you're giving are aloof at best, and bordering on arrogant. It smacks of "my work is perfect; prove to me that you're right", which is not a useful way to communicate with potential users, regardless of license status. Being dismissive of useful information does not convey a tone of a superior product; rather, it's quite the opposite.

I described the problem with a code snippet included, used simple and concise English to point out the concept that needs to be fixed, and in a better communication atmosphere could be convinced to write a patch. (The problem likely isn't difficult.) I'm not new to the software development game by a long shot, but I do try to keep a positive forward-looking perspective.

I understand that everybody has bad days. Would you like to start over?
]
Andy Jefferson made changes - 20/Feb/10 07:37 AM
Comment [ I fail to see how asking for people to follow a simple testcase format is somehow "rude", after all you're the one using the software and docs for free and also the one requiring the fix. So please, one final time, here is the docs link
http://www.datanucleus.org/project/problem_jpa_testcase.html
that means that you can provide a simple zip file following that very simple format that I can drop into my environment and see the issue with a click of the mouse. ]
Andy Jefferson made changes - 20/Feb/10 07:37 AM
Comment [ The code supplied wasn't enough to consider this a 'complete' report? (Read: You don't have to be rude.)

Very well; here's a complete entity that simply wraps the same code I already provided:

=====
@Entity
public class ColumnDefTestEntity {
public static void main(String[] args) throws Exception {
EntityManagerFactory emf = Persistence
.createEntityManagerFactory("jpademo");
EntityManager em = emf.createEntityManager();
EntityTransaction tx = em.getTransaction();
em.persist(new ColumnDefTestEntity());
tx.commit();
em.close();
}

@Id
@GeneratedValue
private Integer id;

@Basic
@Column(columnDefinition = "numeric(5,2) default 1.01")
private float variance;
}
=====

...then see it fail at commit with:

=====
Feb 18, 2010 11:56:22 PM org.datanucleus.store.rdbms.table.AbstractTable create
INFO: Creating table "ColumnDefTestEntity"
Feb 18, 2010 11:56:22 PM org.datanucleus.store.rdbms.table.AbstractTable executeDdlStatementList
SEVERE: Error thrown executing CREATE TABLE "ColumnDefTestEntity"
(
    "id" int4 NOT NULL,
    "variance" float8 NULL numeric(5,2) default 1.01,
    PRIMARY KEY ("id")
) : ERROR: syntax error at or near "numeric"
  Position: 89
org.postgresql.util.PSQLException: ERROR: syntax error at or near "numeric"
  Position: 89
at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2062)
=====

And you'll notice that the column was defined as 'float8 NULL numeric(5,2) default 1.01', obviously not the intent.

By contrast, J2EE works properly with this annotation and produces a column of type NUMERIC(5,2) on PostgreSQL and DECIMAL(5,2) on MySQL. All extant examples I have been able to find on the use of columnDefinition suggest that it is a complete SQL type for the column, replacing any inferred type information. For instance:

http://www.oracle.com/technology/products/ias/toplink/jpa/resources/toplink-jpa-annotations.html#Lob

has a use of @Column(columnDefinition) with a completely specified DDL type, essentially all the information that would normally follow the column name. ]
Andy Jefferson made changes - 20/Feb/10 07:38 AM
Comment [ No testcase, so downgrading ]
Andy Jefferson made changes - 20/Feb/10 07:38 AM
Comment [ Testcase, as per the docs etc etc ? Patch ? ]
Andy Jefferson added a comment - 20/Feb/10 07:42 AM - edited
4 comments deleted since not adding anything constructive to any situation.

The front page of JIRA (Dashboard) mentions the need for a testcase to demonstrate issues (to avoid any possible confusion in expected and actual behaviour). The datanucleus.org docs also mention this. The DataNucleus forum also mentions this.
 
You can choose to not provide a testcase if you so wish (due to time reasons, or whatever), but at the end of the day when someone comes along here in *their spare time* and decides which issue to focus on, one that has a testcase meeting the projects own defined guidelines (and every project has a perfect right to define their own guidelines for what is a testcase) will clearly be chosen first; so its an opportunity for the reporter to help out, and to get the issue that they have met have a higher priority. No problem if you don't provide a testcase matching this, and no issue with necessary basic info provided in the report is rejected for absence of testcase, just that it gets looked at later.

Thanks for the report.

Andy Jefferson added a comment - 14/Apr/10 01:47 PM
SVN trunk aligns the use of columnDefinition with the alternative interpretation of the JPA spec

Andy Jefferson made changes - 14/Apr/10 01:47 PM
Status Open [ 1 ] Resolved [ 5 ]
Fix Version/s 2.1.0.m2 [ 10923 ]
Resolution Fixed [ 1 ]
Andy Jefferson made changes - 26/Apr/10 03:40 PM
Status Resolved [ 5 ] Closed [ 6 ]