You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@syncope.apache.org by GitBox <gi...@apache.org> on 2020/09/08 14:56:01 UTC
[GitHub] [syncope] andrea-patricelli opened a new pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
andrea-patricelli opened a new pull request #213:
URL: https://github.com/apache/syncope/pull/213
This is the fog-catcher implementation for "Propagation task executions and no-SQL persistence", that includes:
- Refactoring of persistence layer to have a table for each task type (push, pull, propagation, notification, sched) with related SQL script to upgrade from previous versions.
- Persistence of PropagationTasks on Elasticsearch as per any objects.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r491943937
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/AbstractSchedTask.java
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.task;
+
+import java.util.Date;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.OneToOne;
+import javax.persistence.Temporal;
+import javax.persistence.TemporalType;
+import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.lib.types.ImplementationType;
+import org.apache.syncope.core.persistence.api.entity.Implementation;
+import org.apache.syncope.core.persistence.api.entity.task.SchedTask;
+import org.apache.syncope.core.persistence.jpa.entity.JPAImplementation;
+
+@MappedSuperclass
+public abstract class AbstractSchedTask extends AbstractTask implements SchedTask {
Review comment:
So each class that extends `AbstractSchedTask` has to define sched task (`JPAPullTask`, `JPAPushTask`, `JPASchedTask`) specific attributes and implement specific methods?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r491933148
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -436,7 +413,13 @@ public int count(
@Transactional(rollbackFor = { Throwable.class })
@Override
public <T extends Task> T save(final T task) {
- return entityManager().merge(task);
+ T merged = entityManager().merge(task);
+
+ // propagate the event only for Propagation tasks
+ if (merged instanceof PropagationTask) {
+ publisher.publishEvent(new TaskCreatedUpdatedEvent<>(this, merged, AuthContextUtils.getDomain()));
Review comment:
It does not make much sense to leave open about things that will never be invoked, as the only caller can realistically be `JPATaskDAO`, and a clear `if` is placed there around publisher invocation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r491943937
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/AbstractSchedTask.java
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.task;
+
+import java.util.Date;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.OneToOne;
+import javax.persistence.Temporal;
+import javax.persistence.TemporalType;
+import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.lib.types.ImplementationType;
+import org.apache.syncope.core.persistence.api.entity.Implementation;
+import org.apache.syncope.core.persistence.api.entity.task.SchedTask;
+import org.apache.syncope.core.persistence.jpa.entity.JPAImplementation;
+
+@MappedSuperclass
+public abstract class AbstractSchedTask extends AbstractTask implements SchedTask {
Review comment:
So each class that extends `AbstractSchedTask` (`JPAPullTask`, `JPAPushTask`, `JPASchedTask`) has to define sched task specific attributes and implement specific methods?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r492538792
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -167,11 +141,11 @@ private String getEntityTableName(final TaskType type) {
return query.getResultList();
}
- private <T extends Task> StringBuilder buildFindAllQueryJPA(final TaskType type) {
+ private <T extends Task> StringBuilder buildFindAllQueryJPA(final Class<T> reference) {
StringBuilder builder = new StringBuilder("SELECT t FROM ").
- append(getEntityReference(type).getSimpleName()).
+ append(getEntityReference(reference).getSimpleName()).
append(" t WHERE ");
- if (type == TaskType.SCHEDULED) {
+ if (SchedTask.class.isAssignableFrom(reference)) {
Review comment:
Ok, going to replace with `JPASchedTask`
##########
File path: core/logic/src/main/java/org/apache/syncope/core/logic/SyncopeLogic.java
##########
@@ -448,7 +448,7 @@ public NumbersInfo numbers() {
numbersInfo.getConfCompleteness().put(
NumbersInfo.ConfItem.NOTIFICATION.name(), !notificationDAO.findAll().isEmpty());
numbersInfo.getConfCompleteness().put(
- NumbersInfo.ConfItem.PULL_TASK.name(), !taskDAO.findAll(TaskType.PULL).isEmpty());
Review comment:
We cannot remove `TaskType` enum, it is necessary for search and to build `TaskQuery` objects.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r491950700
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/AbstractSchedTask.java
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.task;
+
+import java.util.Date;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.OneToOne;
+import javax.persistence.Temporal;
+import javax.persistence.TemporalType;
+import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.lib.types.ImplementationType;
+import org.apache.syncope.core.persistence.api.entity.Implementation;
+import org.apache.syncope.core.persistence.api.entity.task.SchedTask;
+import org.apache.syncope.core.persistence.jpa.entity.JPAImplementation;
+
+@MappedSuperclass
+public abstract class AbstractSchedTask extends AbstractTask implements SchedTask {
Review comment:
No, my comment as above is just about removing `implements SchedTask ` from `AbstractSchedTask`, since, in the same inheritance tree, there is a subclass, e.g. `JPASchedTask` implementing the same interface - not very clean.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r491930178
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -436,7 +413,13 @@ public int count(
@Transactional(rollbackFor = { Throwable.class })
@Override
public <T extends Task> T save(final T task) {
- return entityManager().merge(task);
+ T merged = entityManager().merge(task);
+
+ // propagate the event only for Propagation tasks
+ if (merged instanceof PropagationTask) {
+ publisher.publishEvent(new TaskCreatedUpdatedEvent<>(this, merged, AuthContextUtils.getDomain()));
Review comment:
Yes, I left the implementation in a way "open" also to other tasks. There are some cases in which would be useful also to have `NotificationTask` on ES.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r491933148
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -436,7 +413,13 @@ public int count(
@Transactional(rollbackFor = { Throwable.class })
@Override
public <T extends Task> T save(final T task) {
- return entityManager().merge(task);
+ T merged = entityManager().merge(task);
+
+ // propagate the event only for Propagation tasks
+ if (merged instanceof PropagationTask) {
+ publisher.publishEvent(new TaskCreatedUpdatedEvent<>(this, merged, AuthContextUtils.getDomain()));
Review comment:
It does not make much sense to leave open about things that will never be invoked, as the only caller can realistically be `JPATaskDAO`, and a clear `if` is placed there around publisher invocation.
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/AbstractSchedTask.java
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.task;
+
+import java.util.Date;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.OneToOne;
+import javax.persistence.Temporal;
+import javax.persistence.TemporalType;
+import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.lib.types.ImplementationType;
+import org.apache.syncope.core.persistence.api.entity.Implementation;
+import org.apache.syncope.core.persistence.api.entity.task.SchedTask;
+import org.apache.syncope.core.persistence.jpa.entity.JPAImplementation;
+
+@MappedSuperclass
+public abstract class AbstractSchedTask extends AbstractTask implements SchedTask {
Review comment:
No, my comment as above is just about removing `implements SchedTask ` from `AbstractSchedTask`, since, in the same inheritance tree, there is a subclass, e.g. `JPASchedTask` implementing the same interface - not very clean.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r491926651
##########
File path: core/logic/src/main/java/org/apache/syncope/core/logic/SyncopeLogic.java
##########
@@ -448,7 +448,7 @@ public NumbersInfo numbers() {
numbersInfo.getConfCompleteness().put(
NumbersInfo.ConfItem.NOTIFICATION.name(), !notificationDAO.findAll().isEmpty());
numbersInfo.getConfCompleteness().put(
- NumbersInfo.ConfItem.PULL_TASK.name(), !taskDAO.findAll(TaskType.PULL).isEmpty());
Review comment:
Nope, going to remove it.
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -436,7 +413,13 @@ public int count(
@Transactional(rollbackFor = { Throwable.class })
@Override
public <T extends Task> T save(final T task) {
- return entityManager().merge(task);
+ T merged = entityManager().merge(task);
+
+ // propagate the event only for Propagation tasks
+ if (merged instanceof PropagationTask) {
+ publisher.publishEvent(new TaskCreatedUpdatedEvent<>(this, merged, AuthContextUtils.getDomain()));
Review comment:
Yes, I left the implementation in a way "open" also to other tasks. There are some cases in which would be useful also to have `NotificationTask` on ES.
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/AbstractSchedTask.java
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.task;
+
+import java.util.Date;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.OneToOne;
+import javax.persistence.Temporal;
+import javax.persistence.TemporalType;
+import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.lib.types.ImplementationType;
+import org.apache.syncope.core.persistence.api.entity.Implementation;
+import org.apache.syncope.core.persistence.api.entity.task.SchedTask;
+import org.apache.syncope.core.persistence.jpa.entity.JPAImplementation;
+
+@MappedSuperclass
+public abstract class AbstractSchedTask extends AbstractTask implements SchedTask {
Review comment:
So each class that extends `AbstractSchedTask` has to define sched task (`JPAPullTask`, `JPAPushTask`, `JPASchedTask`) specific attributes and implement specific methods?
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/AbstractSchedTask.java
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.task;
+
+import java.util.Date;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.OneToOne;
+import javax.persistence.Temporal;
+import javax.persistence.TemporalType;
+import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.lib.types.ImplementationType;
+import org.apache.syncope.core.persistence.api.entity.Implementation;
+import org.apache.syncope.core.persistence.api.entity.task.SchedTask;
+import org.apache.syncope.core.persistence.jpa.entity.JPAImplementation;
+
+@MappedSuperclass
+public abstract class AbstractSchedTask extends AbstractTask implements SchedTask {
Review comment:
So each class that extends `AbstractSchedTask` (`JPAPullTask`, `JPAPushTask`, `JPASchedTask`) has to define sched task specific attributes and implement specific methods?
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/AbstractSchedTask.java
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.task;
+
+import java.util.Date;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.OneToOne;
+import javax.persistence.Temporal;
+import javax.persistence.TemporalType;
+import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.lib.types.ImplementationType;
+import org.apache.syncope.core.persistence.api.entity.Implementation;
+import org.apache.syncope.core.persistence.api.entity.task.SchedTask;
+import org.apache.syncope.core.persistence.jpa.entity.JPAImplementation;
+
+@MappedSuperclass
+public abstract class AbstractSchedTask extends AbstractTask implements SchedTask {
Review comment:
Oh ok, now I got. Yes, I agree, I'll leave it in `AbstractSchedTask` and remove in `JPASchedTask.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r492538792
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -167,11 +141,11 @@ private String getEntityTableName(final TaskType type) {
return query.getResultList();
}
- private <T extends Task> StringBuilder buildFindAllQueryJPA(final TaskType type) {
+ private <T extends Task> StringBuilder buildFindAllQueryJPA(final Class<T> reference) {
StringBuilder builder = new StringBuilder("SELECT t FROM ").
- append(getEntityReference(type).getSimpleName()).
+ append(getEntityReference(reference).getSimpleName()).
append(" t WHERE ");
- if (type == TaskType.SCHEDULED) {
+ if (SchedTask.class.isAssignableFrom(reference)) {
Review comment:
Ok, going to replace with `JPASchedTask`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r491926651
##########
File path: core/logic/src/main/java/org/apache/syncope/core/logic/SyncopeLogic.java
##########
@@ -448,7 +448,7 @@ public NumbersInfo numbers() {
numbersInfo.getConfCompleteness().put(
NumbersInfo.ConfItem.NOTIFICATION.name(), !notificationDAO.findAll().isEmpty());
numbersInfo.getConfCompleteness().put(
- NumbersInfo.ConfItem.PULL_TASK.name(), !taskDAO.findAll(TaskType.PULL).isEmpty());
Review comment:
Nope, going to remove it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli edited a comment on pull request #213: SYNCOPE-1592: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli edited a comment on pull request #213:
URL: https://github.com/apache/syncope/pull/213#issuecomment-895288209
> Besides the comments I have added, there are two additional things to cover:
>
> 1. run the tests for `core/persistence-jpa-json` will likely lead to failures, as the `MasterContent.xml` there was not updated
This has been solved.
> 2. at least a test case should be added in `ElasticSearchITCase` to cover the new features
I thought that, as per user search, just current Task IT test were enough, since we simply provide a new implementation for task search.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on pull request #213: SYNCOPE-1592: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on pull request #213:
URL: https://github.com/apache/syncope/pull/213#issuecomment-895288209
> Besides the comments I have added, there are two additional things to cover:
>
> 1. run the tests for `core/persistence-jpa-json` will likely lead to failures, as the `MasterContent.xml` there was not updated
This has been solved.
> 2. at least a test case should be added in `ElasticSearchITCase` to cover the new features
I thought that, as per user search, just current Task IT test were enough, since we simply provide a new implementation for task search.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@syncope.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r492555193
##########
File path: core/logic/src/main/java/org/apache/syncope/core/logic/SyncopeLogic.java
##########
@@ -448,7 +448,7 @@ public NumbersInfo numbers() {
numbersInfo.getConfCompleteness().put(
NumbersInfo.ConfItem.NOTIFICATION.name(), !notificationDAO.findAll().isEmpty());
numbersInfo.getConfCompleteness().put(
- NumbersInfo.ConfItem.PULL_TASK.name(), !taskDAO.findAll(TaskType.PULL).isEmpty());
Review comment:
We cannot remove `TaskType` enum, it is necessary for search and to build `TaskQuery` objects.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r500300174
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/JPATaskExec.java
##########
@@ -42,6 +43,7 @@
* The referred task.
*/
@ManyToOne(optional = false)
+ @JoinColumn(name = "task_id", nullable = false)
Review comment:
Yes, this is needed. Without this the tests `org.apache.syncope.core.persistence.jpa.outer.TaskTest` are going to fail with errors like `Column "TASKEXEC.TASK_ID" not found;`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] ilgrosso commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
ilgrosso commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r485375660
##########
File path: client/console/src/main/resources/org/apache/syncope/client/console/panels/GroupDirectoryPanel_fr_CA.properties
##########
@@ -16,3 +16,4 @@
# under the License.
any.edit=Modifier ${anyTO.type} ${anyTO.name}
group.members=${right} membres de ${left.name}
+auditHistory.title=${anyTO.type} ${anyTO.fullGroupName != "-" ? anyTO.fullGroupName : anyTO.name} histoire
Review comment:
Please do a straight commit for this fix, both for `2_1_X` and `master`, no need to include it in this PR.
##########
File path: core/logic/src/main/java/org/apache/syncope/core/logic/SyncopeLogic.java
##########
@@ -448,7 +448,7 @@ public NumbersInfo numbers() {
numbersInfo.getConfCompleteness().put(
NumbersInfo.ConfItem.NOTIFICATION.name(), !notificationDAO.findAll().isEmpty());
numbersInfo.getConfCompleteness().put(
- NumbersInfo.ConfItem.PULL_TASK.name(), !taskDAO.findAll(TaskType.PULL).isEmpty());
Review comment:
Did you remove the `TaskType` enum completely, right?
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/JPATaskExec.java
##########
@@ -42,6 +43,7 @@
* The referred task.
*/
@ManyToOne(optional = false)
+ @JoinColumn(name = "task_id", nullable = false)
Review comment:
Is this `@JoinColumn` annotation really needed? What if you remove it?
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -458,11 +441,12 @@ public void delete(final Task task) {
}
entityManager().remove(task);
+ publisher.publishEvent(new TaskDeletedEvent(this, task.getKey(), AuthContextUtils.getDomain()));
Review comment:
Why not doing this only for propagation tasks, as done above, instead?
##########
File path: fit/core-reference/pom.xml
##########
@@ -443,6 +443,74 @@ under the License.
</build>
<profiles>
+ <profile>
Review comment:
Please revert the changes to this file, they are not needed.
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -167,11 +141,11 @@ private String getEntityTableName(final TaskType type) {
return query.getResultList();
}
- private <T extends Task> StringBuilder buildFindAllQueryJPA(final TaskType type) {
+ private <T extends Task> StringBuilder buildFindAllQueryJPA(final Class<T> reference) {
StringBuilder builder = new StringBuilder("SELECT t FROM ").
- append(getEntityReference(type).getSimpleName()).
+ append(getEntityReference(reference).getSimpleName()).
append(" t WHERE ");
- if (type == TaskType.SCHEDULED) {
+ if (SchedTask.class.isAssignableFrom(reference)) {
Review comment:
This is not correct, as `type == TaskType.SCHEDULE` used to match only `SchedTask` while now this is also matching `PushTask`, `PullTask` and so on.
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -216,84 +190,86 @@ private StringBuilder buildFindAllQuery(
final List<Object> queryParameters) {
if (resource != null
- && type != TaskType.PROPAGATION && type != TaskType.PUSH && type != TaskType.PULL) {
+ && !PropagationTask.class.isAssignableFrom(reference)
+ && !PushTask.class.isAssignableFrom(reference)
+ && !PullTask.class.isAssignableFrom(reference)) {
- throw new IllegalArgumentException(type + " is not related to " + ExternalResource.class.getSimpleName());
+ throw new IllegalArgumentException(reference.getSimpleName() + " is not related to "
+ + ExternalResource.class.getSimpleName());
}
if ((anyTypeKind != null || entityKey != null)
- && type != TaskType.PROPAGATION && type != TaskType.NOTIFICATION) {
+ && !PropagationTask.class.isAssignableFrom(reference)
+ && !NotificationTask.class.isAssignableFrom(reference)) {
- throw new IllegalArgumentException(type + " is not related to users, groups or any objects");
+ throw new IllegalArgumentException(reference.getSimpleName()
+ + " is not related to users, groups or any objects");
}
- if (notification != null && type != TaskType.NOTIFICATION) {
- throw new IllegalArgumentException(type + " is not related to notifications");
+ if (notification != null && !NotificationTask.class.isAssignableFrom(reference)) {
+ throw new IllegalArgumentException(reference.getSimpleName() + " is not related to notifications");
}
- StringBuilder queryString = new StringBuilder("SELECT ").append(AbstractTask.TABLE).append(".*");
+ String tableName = getEntityTableName(reference);
+ StringBuilder queryString = new StringBuilder("SELECT ").append(tableName).append(".*");
if (orderByTaskExecInfo) {
queryString.append(",").append(JPATaskExec.TABLE).append(".startDate AS startDate").
append(",").append(JPATaskExec.TABLE).append(".endDate AS endDate").
append(",").append(JPATaskExec.TABLE).append(".status AS status").
- append(" FROM ").append(AbstractTask.TABLE).
+ append(" FROM ").append(tableName).
append(",").append(JPATaskExec.TABLE).append(",").append("(SELECT ").
append(JPATaskExec.TABLE).append(".task_id, ").
append("MAX(").append(JPATaskExec.TABLE).append(".startDate) AS startDate").
append(" FROM ").append(JPATaskExec.TABLE).
append(" GROUP BY ").append(JPATaskExec.TABLE).append(".task_id) GRP").
append(" WHERE ").
- append(AbstractTask.TABLE).append(".id=").append(JPATaskExec.TABLE).append(".task_id").
- append(" AND ").append(AbstractTask.TABLE).append(".id=").append("GRP.task_id").
- append(" AND ").append(JPATaskExec.TABLE).append(".startDate=").append("GRP.startDate").
- append(" AND ").append(AbstractTask.TABLE).append(".DTYPE = ?1");
+ append(tableName).append(".id=").append(JPATaskExec.TABLE).append(".task_id").
+ append(" AND ").append(tableName).append(".id=").append("GRP.task_id").
+ append(" AND ").append(JPATaskExec.TABLE).append(".startDate=").append("GRP.startDate");
} else {
- queryString.append(", null AS startDate, null AS endDate, null AS status FROM ").append(AbstractTask.TABLE).
- append(" WHERE ").append(AbstractTask.TABLE).append(".DTYPE = ?1");
+ queryString.
+ append(", null AS startDate, null AS endDate, null AS status FROM ").
+ append(tableName).
+ append(" WHERE 1=1");
}
- queryParameters.add(getEntityTableName(type));
- if (type == TaskType.SCHEDULED) {
+ queryParameters.add(getEntityTableName(reference));
+ if (SchedTask.class.isAssignableFrom(reference)) {
Review comment:
Same here as above.
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPATaskDAO.java
##########
@@ -436,7 +413,13 @@ public int count(
@Transactional(rollbackFor = { Throwable.class })
@Override
public <T extends Task> T save(final T task) {
- return entityManager().merge(task);
+ T merged = entityManager().merge(task);
+
+ // propagate the event only for Propagation tasks
+ if (merged instanceof PropagationTask) {
+ publisher.publishEvent(new TaskCreatedUpdatedEvent<>(this, merged, AuthContextUtils.getDomain()));
Review comment:
Maybe `TaskCreatedUpdatedEvent` shall be renamed as `PropagationTaskCreatedUpdatedEvent`?
##########
File path: fit/console-reference/pom.xml
##########
@@ -276,6 +276,7 @@ under the License.
<properties>
<skipTests>true</skipTests>
+ <docker.autoCreateCustomNetworks>true</docker.autoCreateCustomNetworks>
Review comment:
Please revert the changes to this file, they are not needed.
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/AbstractSchedTask.java
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.task;
+
+import java.util.Date;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.OneToOne;
+import javax.persistence.Temporal;
+import javax.persistence.TemporalType;
+import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.lib.types.ImplementationType;
+import org.apache.syncope.core.persistence.api.entity.Implementation;
+import org.apache.syncope.core.persistence.api.entity.task.SchedTask;
+import org.apache.syncope.core.persistence.jpa.entity.JPAImplementation;
+
+@MappedSuperclass
+public abstract class AbstractSchedTask extends AbstractTask implements SchedTask {
Review comment:
Since `JPASchedTask` implements `SchedTask`, I think it would be cleaner to remove `implements SchedTask` from here.
##########
File path: core/persistence-jpa/src/test/resources/domains/Master.properties
##########
@@ -16,6 +16,7 @@
# under the License.
Master.driverClassName=org.h2.Driver
Master.url=jdbc:h2:mem:syncopedb;DB_CLOSE_DELAY=-1
+#Master.url=jdbc:h2:file:~/syncopedb;DB_CLOSE_ON_EXIT=FALSE;AUTO_RECONNECT=TRUE
Review comment:
Please revert this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [syncope] andrea-patricelli commented on a change in pull request #213: Fog catcher: Propagation task executions and no-SQL persistence
Posted by GitBox <gi...@apache.org>.
andrea-patricelli commented on a change in pull request #213:
URL: https://github.com/apache/syncope/pull/213#discussion_r491952338
##########
File path: core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/task/AbstractSchedTask.java
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.jpa.entity.task;
+
+import java.util.Date;
+import javax.persistence.MappedSuperclass;
+import javax.persistence.OneToOne;
+import javax.persistence.Temporal;
+import javax.persistence.TemporalType;
+import javax.validation.constraints.NotNull;
+import org.apache.syncope.common.lib.types.ImplementationType;
+import org.apache.syncope.core.persistence.api.entity.Implementation;
+import org.apache.syncope.core.persistence.api.entity.task.SchedTask;
+import org.apache.syncope.core.persistence.jpa.entity.JPAImplementation;
+
+@MappedSuperclass
+public abstract class AbstractSchedTask extends AbstractTask implements SchedTask {
Review comment:
Oh ok, now I got. Yes, I agree, I'll leave it in `AbstractSchedTask` and remove in `JPASchedTask.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org