kaleidos / grails-postgresql-extensions

Grails plugin to use postgresql native elements such as arrays, hstores,...
Apache License 2.0
78 stars 62 forks source link

HstoreHelper toMap throws exception if the key or the value contains a , #20

Closed mkobel closed 10 years ago

mkobel commented 10 years ago

The .split(", ") in src/java/net/kaleidos/hibernate/usertype/HstoreHelper.java @ Line 74 does also split keys and values containing a ,

ilopmar commented 10 years ago

I've added some tests with a "comma" in keys and values but all the tests pass. I've improved the tests, you can see in this commit 7a854a7a31ea537989f6059fbb1d6206e2c14ac6.

Can you run the tests now and check if all pass? If not, could you provide more information: environment, grails version, postgresql version,...

Regards, Iván.

mkobel commented 10 years ago

Your new test run on my system without error.

This is the error I get in my application:

Caused by ArrayIndexOutOfBoundsException: 1 ->> 79 | toMap in net.kaleidos.hibernate.usertype.HstoreHelper

When I replace , by another value before saving, then it works.

One of the values contains this one: "Orientation"=>"Top, left side (Horizontal / normal)"

Grails: 2.2.4 PostgreSQL: 9.3.1 on Arch Linux JDBC: 'postgresql:postgresql:9.0-901.jdbc4'

I will now try to improve your tests.

mkobel commented 10 years ago

In my commit https://github.com/mkobel/grails-postgresql-extensions/commit/e613dfc416974130ed8f49e082b1c71a7e8597b7 I added two tests. The second one fails.

mkobel commented 10 years ago

This code could help to improve the parser: https://code.google.com/p/pg-spring-type-mapper/source/browse/src/main/java/org/valgog/utils/postgres/HStore.java

What do you think about it?

ilopmar commented 10 years ago

Hi, we're going to analyze this because it's not simple.

mkobel commented 10 years ago

According to my understanding of the problem, the parser should not simply split at COMMA-SPACE. Therefore a more complex parser should be used, as you can find them searching for other HStore-parser implementations.

Would you like to work on this topic yourself or should I work on this too?

ilopmar commented 10 years ago

We don't know when we are going to find some spare time to work on this. Maybe later this week or the following weekend. If you work on this with the parser you have suggested (or another one) we'll be very happy to accept the PR.

Regards, Iván.

mkobel commented 10 years ago

Hello Iván,

I just completed an implementation based on the code from pg-spring-type-mapper which passes all tests! https://github.com/mkobel/grails-postgresql-extensions/commit/3373c96435e052097a8501461f2d0ecf69a4abb2

Would you like to make a review before I create a PR?

Best regards, Moritz

ilopmar commented 10 years ago

Hello Moritz,

I think everything it's ok, great work :-). Please create the PR.

I've a question. Is it possible to save values with double-quotes with this new parser?

Regards, Iván.

ilopmar commented 10 years ago

Fixex in #22