[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[IsurangaPerera]

Github user IsurangaPerera closed the pull request at:

https://github.com/apache/syncope/pull/70


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181103559
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAccessTokenDAO.java
 ---
@@ -115,6 +115,16 @@ public AccessToken save(final AccessToken accessToken) 
{
 return entityManager().merge(accessToken);
 }
 
+@Override
+public AccessToken replace(final AccessToken accessToken) {
+AccessToken existing = findByOwner(accessToken.getOwner());
+if (existing != null) {
+delete(existing.getKey());
+}
+
+return save(accessToken);
--- End diff --

`return replaceExisting ? save(accessToken) : existing;`


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181103181
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAccessTokenDAO.java
 ---
@@ -115,6 +115,16 @@ public AccessToken save(final AccessToken accessToken) 
{
 return entityManager().merge(accessToken);
 }
 
+@Override
+public AccessToken replace(final AccessToken accessToken) {
+AccessToken existing = findByOwner(accessToken.getOwner());
+if (existing != null) {
--- End diff --

`if (replaceExisting && existing != null) {`


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181102107
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

both: remove `unique = true` (e.g. revert all changes there, as said) AND 
keep the same code


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[IsurangaPerera]

Github user IsurangaPerera commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181101464
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

so you're suggesting keep this code segment intact right?
if (replaceExisting && existing != null) {
accessTokenDAO.delete(existing);
}

or remove unique = true from the model?


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181100854
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/JPAAccessToken.java
 ---
@@ -44,7 +44,7 @@
 @Temporal(TemporalType.TIMESTAMP)
 private Date expiryTime;
 
-@Column(nullable = true)
+@Column(nullable = true, unique = true)
--- End diff --

as said, revert any change here


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181100663
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAccessTokenDAO.java
 ---
@@ -115,6 +115,16 @@ public AccessToken save(final AccessToken accessToken) 
{
 return entityManager().merge(accessToken);
 }
 
+@Override
+public AccessToken replace(final AccessToken accessToken) {
+AccessToken existing = findByOwner(accessToken.getOwner());
+if (existing != null) {
+delete(existing.getKey());
+}
+
+return entityManager().merge(accessToken);
--- End diff --

replace `entityManager().merge(accessToken)` with `save(accesToken)`


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181100023
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

That's why we are not enforcing the unique constraint (hence, NOT changing 
`@Column(nullable = true)`: the `replaceExisting` flag exists for a reason, do 
not alter such behavior as there might be some code relying on it.


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[IsurangaPerera]

Github user IsurangaPerera commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181099396
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

Actually that way there can be 2 tokens at a given moment. Suppose there 
exist an access token already. when trying to replace it in the old way it 
creates and saves another token (at this time there is 2 token which is against 
the unique constraint). 


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181097338
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

To me, it should be enough to pass the `replaceExisting` flag to the new 
`AccessTokenDAO#replace` method, and behave accordingly.


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181096467
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

...but there might be cases where such constraint should not apply (see the 
`@Column` annotation we've been discussing).


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[IsurangaPerera]

Github user IsurangaPerera commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181094994
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

exactly. this is to assure that there can be only one access token 
associated with a particular user at an any given time and also a workaround to 
overcome thread not safe problem without affecting the performance(without 
using serialized isolation)


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181093676
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAccessTokenDAO.java
 ---
@@ -115,6 +115,16 @@ public AccessToken save(final AccessToken accessToken) 
{
 return entityManager().merge(accessToken);
 }
 
+@Override
+@Transactional(rollbackFor = Throwable.class)
+public void merge(final AccessToken accessToken) {
+AccessToken existing = findByOwner(accessToken.getOwner());
+if (existing != null) {
+entityManager().remove(existing);
+}
+entityManager().persist(accessToken);
--- End diff --

Also, replace `entityManager().remove(existing)` with `delete(existing)`, 
thanks.


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181093708
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

So, the current implementation will replace the token only if 
`replaceExisting` is true; with your change, the token will be replaced anyway, 
because `JPAAccessTokenDAO#merge` will do

```
if (existing != null) {
entityManager().remove(existing);
}
```
while the current code does
```
if (replaceExisting && existing != null) {
accessTokenDAO.delete(existing);
}
```


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181091780
  
--- Diff: 
core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/dao/AccessTokenDAO.java
 ---
@@ -34,6 +34,8 @@
 
 AccessToken save(AccessToken accessToken);
 
+void merge(AccessToken accessToken);
--- End diff --

maybe `replace()` makes more sense


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[IsurangaPerera]

Github user IsurangaPerera commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181091428
  
--- Diff: 
core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/dao/AccessTokenDAO.java
 ---
@@ -34,6 +34,8 @@
 
 AccessToken save(AccessToken accessToken);
 
+void merge(AccessToken accessToken);
--- End diff --

Shall I keep the method name same(merge) or change it?


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[IsurangaPerera]

Github user IsurangaPerera commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181090557
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

It is not completely ignored. it is checked by the second if condition as 
before


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181089858
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/JPAAccessToken.java
 ---
@@ -44,7 +44,7 @@
 @Temporal(TemporalType.TIMESTAMP)
 private Date expiryTime;
 
-@Column(nullable = true)
+@Column(unique = true)
--- End diff --

Just revert, I know `nullable = true` is the default, but I don't see 
reasons to change.


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181089640
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

`replaceExisting` is an argument of `AccesstTokenDataBinderImpl#create` 
that your change would simply ignore.
Not good: it must be checked.


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[IsurangaPerera]

Github user IsurangaPerera commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181089295
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/JPAAccessToken.java
 ---
@@ -44,7 +44,7 @@
 @Temporal(TemporalType.TIMESTAMP)
 private Date expiryTime;
 
-@Column(nullable = true)
+@Column(unique = true)
--- End diff --

I think in JPA it is redundant to use nullable = true this is the default 
scenario or all columns in JPA. Should I still change it?


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181088731
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/JPAAccessToken.java
 ---
@@ -44,7 +44,7 @@
 @Temporal(TemporalType.TIMESTAMP)
 private Date expiryTime;
 
-@Column(nullable = true)
+@Column(unique = true)
--- End diff --

As said during the initial mail exchange, I am not sure of the reason why 
owner is currently set as nullable, and I don't see enough reasons to change 
it, please revert.


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[IsurangaPerera]

Github user IsurangaPerera commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181088841
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

yes, merge function simply ensure the uniqueness constraint by checking if 
the token exists and replace it if so. otherwise, it will create a new 
register. so no need to perform this check here


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181088154
  
--- Diff: 
core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/AccessTokenDataBinderImpl.java
 ---
@@ -135,11 +135,7 @@
 accessToken.setAuthorities(authorities);
 }
 
-accessTokenDAO.save(accessToken);
-}
-
-if (replaceExisting && existing != null) {
-accessTokenDAO.delete(existing);
+accessTokenDAO.merge(accessToken);
--- End diff --

what about the `replaceExistingFlag`? You just removed it!


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181087678
  
--- Diff: 
core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/dao/AccessTokenDAO.java
 ---
@@ -34,6 +34,8 @@
 
 AccessToken save(AccessToken accessToken);
 
+void merge(AccessToken accessToken);
--- End diff --

Change it into
```
AccessToken merge(AccessToken accessToken);
```


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181087221
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAccessTokenDAO.java
 ---
@@ -115,6 +115,16 @@ public AccessToken save(final AccessToken accessToken) 
{
 return entityManager().merge(accessToken);
 }
 
+@Override
+@Transactional(rollbackFor = Throwable.class)
+public void merge(final AccessToken accessToken) {
+AccessToken existing = findByOwner(accessToken.getOwner());
+if (existing != null) {
+entityManager().remove(existing);
+}
+entityManager().persist(accessToken);
--- End diff --

replace with `save(accessToken)` and make this method return `AccessToken` 
as `save()` does.


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[ilgrosso]

Github user ilgrosso commented on a diff in the pull request:

https://github.com/apache/syncope/pull/70#discussion_r181086944
  
--- Diff: 
core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAAccessTokenDAO.java
 ---
@@ -115,6 +115,16 @@ public AccessToken save(final AccessToken accessToken) 
{
 return entityManager().merge(accessToken);
 }
 
+@Override
+@Transactional(rollbackFor = Throwable.class)
--- End diff --

Please remove this annotation, it is redundant..


---

[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed

[IsurangaPerera]

GitHub user IsurangaPerera opened a pull request:

https://github.com/apache/syncope/pull/70

[SYNCOPE-1301] fixed



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/IsurangaPerera/syncope SYNCOPE-1301

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/syncope/pull/70.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #70


commit 0c9d91fa51544467e86d6427d9009306f05b1cde
Author: Isuranga Perera 
Date:   2018-04-12T13:41:46Z

[SYNCOPE-1301] fixed




---