Add JPA implementions of UserDao/ProfileDao #1603

voievodin · 2016-06-29T11:21:04Z

Along with JPA implementations i added 2 password encryptors SHA-512 and PBKDF2.
JpaUserDao and JpaProfileDao pass all the TCK prepared before.

@skabashnyuk, @sleshchenko, @garagatyi, @akorneta can you please review.
Please note that this will be merged into the jpa-integration branch which will be merged to the master when all the DAOs are migrated to JPA.

sleshchenko · 2016-06-29T11:49:27Z

...-vendor-h2/src/main/java/org/eclipse/che/api/core/jdbc/jpa/vendor/h2/H2ExceptionHandler.java

+ */
+public class H2ExceptionHandler implements ExceptionHandler {
+
+    // TODO move it from here


When do you plan remove it?

benoitf · 2016-06-29T12:19:09Z

few remarks:

If there are password encryptors, why they're not part of a separated commits as it's not linked to JPA ?
why the wheel has been re-invented with TCK modules ? while people are using Arquilian when testing JPA ? http://arquillian.org/guides/testing_java_persistence/
- che-core-api-jdbc-vendor-h2
  the name is not obvious as it links the module to EclipseLink, while JPA allow to plug any persistence provider. so it should include that it's specific to eclipselink and is not "jdbc" neutral
persistence.xml shouldn't be in wsmaster/che-core-api-user/src/main/resources/META-INF/
it's part of the final assembly as it's where we select the persistence provider, and the database
Using container managed instead of JavaSE would have helped as here you've to managed transaction instead of only using annotations
Using an autocloseable (or an interceptor like it's done in container managed world) should help to avoid to have tons of lines doing. Then you could pickup the transaction strategy (REQUIRED, REQUIRES_NEW, etc)


try {
            manager.getTransaction().begin();

// do something

           manager.getTransaction().commit();
        } finally {
            if (manager.getTransaction().isActive()) {
               manager.getTransaction().rollback();
            }
            manager.close();
       }

voievodin · 2016-06-29T12:49:38Z

If there are password encryptors, why they're not part of a separated commits as it's not linked to JPA ?

np, i will extract ecnryptors code to the different commit.

why the wheel has been re-invented with TCK modules ? while people are using Arquilian when testing JPA ? http://arquillian.org/guides/testing_java_persistence

Current TCK approach allows us use the same tests for different implementations Local/Ldap/Jpa.
Can Arquilian resolve this?

che-core-api-jdbc-vendor-h2 the name is not obvious as it links the module to EclipseLink, while JPA allow to plug any persistence provider. so it should include that it's specific to eclipselink and is not "jdbc" neutral

che-core-api-jdbc module contains common error codes and error codes mapper astraction used by daos, che-core-api-jdbc-vendor-h2 provides an implementation of error codes mapper specific for h2, while error codes are jdbc errors.
I will move code specific to eclipse link to the dedicated jpa/eclipselink package are you okay with that?

persistence.xml shouldn't be in wsmaster/che-core-api-user/src/main/resources/META-INF/
it's part of the final assembly as it's where we select the persistence provider, and the database

okay thank you, i am using it for test purposes only, so i will remove it from there.

Using container managed instead of JavaSE would have helped as here you've to managed transaction instead of only using annotations
Using an autocloseable (or an interceptor like it's done in container managed world) should help to avoid to have tons of lines doing. Then you could pickup the transaction strategy (REQUIRED, REQUIRES_NEW, etc)

We will use this annotation, i will introduce it with next pull request, right now alot of things depend on this pull request that's why we decided to merge it asap which allows me and guys to work on jpa implementations separately.

benoitf · 2016-06-29T13:18:22Z

If there are password encryptors, why they're not part of a separated commits as it's not linked to JPA ?

np, i will extract ecnryptors code to the different commit.

thx

why the wheel has been re-invented with TCK modules ? while people are using Arquilian when testing JPA ? http://arquillian.org/guides/testing_java_persistence

Current TCK approach allows us use the same tests for different implementations Local/Ldap/Jpa.
Can Arquilian resolve this?

It allows to test with different JPA providers and different database as well
also it's a Container Agnostic framework and allow plugability.

che-core-api-jdbc-vendor-h2 the name is not obvious as it links the module to EclipseLink, while JPA allow to plug any persistence provider. so it should include that it's specific to eclipselink and is not "jdbc" neutral

che-core-api-jdbc module contains common error codes and error codes mapper astraction used by >daos, che-core-api-jdbc-vendor-h2 provides an implementation of error codes mapper specific for h2, >while error codes are jdbc errors.
I will move code specific to eclipse link to the dedicated jpa/eclipselink package are you okay with >that?

sure

persistence.xml shouldn't be in wsmaster/che-core-api-user/src/main/resources/META-INF/
it's part of the final assembly as it's where we select the persistence provider, and the database

okay thank you, i am using it for test purposes only, so i will remove it from there.

ok

Using container managed instead of JavaSE would have helped as here you've to managed >>transaction instead of only using annotations
Using an autocloseable (or an interceptor like it's done in container managed world) should help to >>avoid to have tons of lines doing. Then you could pickup the transaction strategy (REQUIRED, >>REQUIRES_NEW, etc)

We will use this annotation, i will introduce it with >next pull request, right now alot of things depend on this pull request that's why we decided to merge it >asap which allows me and guys to work on jpa implementations separately.

Yes this Guice stuff or better, CDI, which offer more stuff around Java EE component will allow more possibilities around PersistenceContext

codenvy-ci · 2016-06-29T17:10:42Z

Build success. http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/1056/

garagatyi · 2016-06-30T06:45:15Z

core/che-core-api-core/src/main/java/org/eclipse/che/security/PBKDF2PasswordEncryptor.java

+        final Integer iterations = tryParse(firstNonNull(matcher.group("iterations"), ""));
+        final String salt = matcher.group("saltHash");
+        final String pwdHash = matcher.group("pwdHash");
+        if (salt == null || iterations == null || iterations <= 0 || pwdHash == null) {


Encrypted password matches regexp and regexp require 1+ symbols in each group. It looks like NULL checks are not needed.

I mean salt and pwdHash

codenvy-ci · 2016-06-30T20:36:43Z

Build success. http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/1083/

voievodin · 2016-07-01T09:02:20Z

Pull request updated, @benoitf @skabashnyuk please check

garagatyi · 2016-07-01T09:35:45Z

core/che-core-api-core/src/main/java/org/eclipse/che/security/PBKDF2PasswordEncryptor.java

+            return false;
+        }
+        // retrieve salt, password hash and iterations count from hash
+        final Integer iterations = tryParse(firstNonNull(matcher.group("iterations"), ""));


I think group can't be Null in that case

yes, thank you!

benoitf · 2016-07-01T10:08:19Z

ok

skabashnyuk · 2016-07-01T14:23:18Z

ok

garagatyi · 2016-07-01T14:23:59Z

LGTM

codenvy-ci · 2016-07-01T14:30:06Z

Build # 1091 - FAILED

Please check console output at http://ci.codenvy-dev.com/jenkins/job/che-pullrequests-build/1091/ to view the results.

sleshchenko reviewed Jun 29, 2016
View reviewed changes

garagatyi reviewed Jun 30, 2016
View reviewed changes

voievodin force-pushed the jpa-integration branch from a9732dc to 1b237b3 Compare June 30, 2016 16:21

voievodin force-pushed the user-jpa branch from 92f7ef2 to 77c60de Compare June 30, 2016 16:41

voievodin force-pushed the jpa-integration branch from 1b237b3 to 4d12541 Compare July 1, 2016 07:06

voievodin force-pushed the user-jpa branch from 77c60de to 4e415c2 Compare July 1, 2016 07:07

Yevhenii Voevodin added 2 commits July 1, 2016 11:39

Add PasswordEncruptor contract and several implementations

5ac834e

Add UserDao/ProfileDao jpa implementations

7158778

voievodin force-pushed the user-jpa branch from 9da4365 to 7158778 Compare July 1, 2016 08:56

garagatyi reviewed Jul 1, 2016
View reviewed changes

fixup! Add PasswordEncruptor contract and several implementations

7766546

voievodin merged commit 47afe20 into jpa-integration Jul 1, 2016

voievodin deleted the user-jpa branch July 1, 2016 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JPA implementions of UserDao/ProfileDao #1603

Add JPA implementions of UserDao/ProfileDao #1603

voievodin commented Jun 29, 2016 •

edited

Loading

sleshchenko Jun 29, 2016

benoitf commented Jun 29, 2016 •

edited

Loading

voievodin commented Jun 29, 2016 •

edited

Loading

benoitf commented Jun 29, 2016

codenvy-ci commented Jun 29, 2016

garagatyi Jun 30, 2016

garagatyi Jun 30, 2016

codenvy-ci commented Jun 30, 2016

voievodin commented Jul 1, 2016

garagatyi Jul 1, 2016

voievodin Jul 1, 2016

benoitf commented Jul 1, 2016

skabashnyuk commented Jul 1, 2016

garagatyi commented Jul 1, 2016

codenvy-ci commented Jul 1, 2016

Add JPA implementions of UserDao/ProfileDao #1603

Add JPA implementions of UserDao/ProfileDao #1603

Conversation

voievodin commented Jun 29, 2016 • edited Loading

sleshchenko Jun 29, 2016

Choose a reason for hiding this comment

benoitf commented Jun 29, 2016 • edited Loading

voievodin commented Jun 29, 2016 • edited Loading

benoitf commented Jun 29, 2016

codenvy-ci commented Jun 29, 2016

garagatyi Jun 30, 2016

Choose a reason for hiding this comment

garagatyi Jun 30, 2016

Choose a reason for hiding this comment

codenvy-ci commented Jun 30, 2016

voievodin commented Jul 1, 2016

garagatyi Jul 1, 2016

Choose a reason for hiding this comment

voievodin Jul 1, 2016

Choose a reason for hiding this comment

benoitf commented Jul 1, 2016

skabashnyuk commented Jul 1, 2016

garagatyi commented Jul 1, 2016

codenvy-ci commented Jul 1, 2016

voievodin commented Jun 29, 2016 •

edited

Loading

benoitf commented Jun 29, 2016 •

edited

Loading

voievodin commented Jun 29, 2016 •

edited

Loading