[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed
Github user IsurangaPerera closed the pull request at: https://github.com/apache/syncope/pull/70 ---
[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed
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
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
Github user ilgrosso commented on a diff in the pull request: https://github.com/apache/syncope/pull/70#discussion_r181103053 --- Diff: core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/dao/AccessTokenDAO.java --- @@ -34,6 +34,8 @@ AccessToken save(AccessToken accessToken); +AccessToken replace(AccessToken accessToken); --- End diff -- This should be ``` AccessToken replace(boolean replaceExisting, AccessToken accessToken); ``` ---
[GitHub] syncope pull request #70: [SYNCOPE-1301] fixed
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 ---