You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/06/10 19:27:47 UTC
[GitHub] [nifi] tpalfy opened a new pull request #5145: NIFI-8671 Create nifi-versioned-components module.
tpalfy opened a new pull request #5145:
URL: https://github.com/apache/nifi/pull/5145
https://issues.apache.org/jira/browse/NIFI-8671
Extract VersionedComponent class family from nifi-registry to the new module nifi-versioned-components.
<!--
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.
-->
Thank you for submitting a contribution to Apache NiFi.
Please provide a short description of the PR here:
#### Description of PR
_Enables X functionality; fixes bug NIFI-YYYY._
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with **NIFI-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [ ] Is your initial contribution a single, squashed commit? _Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not `squash` or use `--force` when pushing to allow for clean monitoring of changes._
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi` folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] Have you verified that the full build is successful on JDK 8?
- [ ] Have you verified that the full build is successful on JDK 11?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-assembly`?
- [ ] If adding new Properties, have you added `.displayName` in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.
--
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] [nifi] tpalfy commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r650882924
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
~~I'll remove the nifi-api -> nifi-versioned-components dependency then.
When I open a new PR for Flow Analysis, I'll have nifi-framework-core depend on it instead.
As suggested, later when an extension needs the VersionedComponent classes, it needs to introduce this dependency as well.
It doesn't seem to be too hard to change later if we need it and now we can move forward.~~
--
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] [nifi] exceptionfactory commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r658819453
##########
File path: nifi-registry/pom.xml
##########
@@ -356,6 +356,11 @@
<artifactId>slf4j-api</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ </dependency>
Review comment:
Adding the swagger-annotations dependency at the parent level appears to be adding it to all modules, however, it seems like it should only be necessary in the `nifi-registry-data-model` module. Is there a reason for including here?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 commented on pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#issuecomment-895537513
Thanks guys for the thorough review & progression here! I think this is looking really good. Keeps the API really nice and clean. I'm a +1 as well. Will merge to main
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r680916610
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/serialization/jaxb/JAXBSerializer.java
##########
@@ -73,8 +79,12 @@ public void serialize(final int dataModelVersion, final T t, final OutputStream
try {
final Marshaller marshaller = jaxbContext.createMarshaller();
marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
- marshaller.marshal(t, out);
- } catch (JAXBException e) {
+
+ String className = clazz.getSimpleName();
+ String tagName = Character.toLowerCase(className.charAt(0)) + className.substring(1);
+
+ marshaller.marshal(new JAXBElement<>(new QName(tagName), clazz, t), out);
Review comment:
I'd rather not pull a direct reference to `VersionedProcessGroup` into this class.
Not a fan of reflection either - I'd stay strongly typed if I can help.
I have a third option: try to marshal the old way, if that doesn't work, fall back to the new way.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r680917399
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/serialization/jaxb/JAXBSerializer.java
##########
@@ -88,9 +98,15 @@ public T deserialize(final InputStream input) throws SerializationException {
try {
// Consume the header bytes.
readDataModelVersion(input);
+
+ XMLStreamReader streamReader = XMLInputFactory.newInstance().createXMLStreamReader(input);
+
final Unmarshaller unmarshaller = jaxbContext.createUnmarshaller();
- return (T) unmarshaller.unmarshal(input);
- } catch (JAXBException e) {
+ JAXBElement<T> jaxbElement = unmarshaller.unmarshal(streamReader, clazz);
+ T deserializedObject = jaxbElement.getValue();
+
+ return deserializedObject;
Review comment:
I find it tremendously useful - especially during debug - to have a single variable as a return value instead of an expression.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r649834938
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
I discussed this dependency issue with @kevdoran and he suggested this -imo- neat idea.
The `provided` scope is to _not_ introduce the swagger dependency to _nifi-api_ build time. With this we can avoid having it on the root classpath.
The run-time burden falls to the referring modules indeed but that shouldn't be a big issue. nifi-registry already had this obligation and the nifi-framework bundle also depends on swagger-annotations.
Considering only 2 high-level api annotations are used in nifi-versioned-components I think this is the best compromise we can get.
As for why even add this change _here_. The change in this PR was originally part of https://github.com/apache/nifi/pull/5118 and I'm now separating it from the Flow Analysis change as you suggested.
So the Flow Analysis change would follow this one soon. And I'd rather have this nifi-api -> nifi-versioned-components added in this one rather than that one as it has more general - we make those classes part of the public API. That is the purpose of this new module in the first place.
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
I discussed this dependency issue with @kevdoran and he suggested this -imo- neat idea.
The `provided` scope is to _not_ introduce the swagger dependency to _nifi-api_ build time. With this we can avoid having it on the root classpath.
The run-time burden falls to the referring modules indeed but that shouldn't be a big issue. nifi-registry already had this obligation and the nifi-framework bundle also depends on swagger-annotations.
Considering only 2 high-level api annotations are used in nifi-versioned-components I think this is the best compromise we can get.
As for why even add this change _here_. The change in this PR was originally part of https://github.com/apache/nifi/pull/5118 and I'm now separating it from the Flow Analysis change as you suggested.
So the Flow Analysis change would follow this one soon. And I'd rather have this nifi-api -> nifi-versioned-components added in this one rather than that one as it has more general implications - we make those classes part of the public API. That is the purpose of this new module in the first place.
--
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] [nifi] tpalfy commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r650882924
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
~~I'll remove the nifi-api -> nifi-versioned-components dependency then.
When I open a new PR for Flow Analysis, I'll have nifi-framework-core depend on it instead.
As suggested, later when an extension needs the VersionedComponent classes, it needs to introduce this dependency as well.
It doesn't seem to be too hard to change later if we need it and now we can move forward.~~
I don't see it feasible to avoid the nifi-api -> nifi-versioned-components dependency.
Sure it would make this PR easier to handle but that would just delay the problem to the immediate next PR: the one that introduces the Flow Analysis. As it needs to add a new interface to nifi-api that depends on the nifi-versioned-components classes.
In fact this PR serves little to no purpose if the nifi-api -> nifi-versioned-components dependency is left out. It's just an oversized refactor then. Making the VersionedComponents available through the api is the _goal_ and the refactor is the _means_. Not the other way around.
--
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] [nifi] exceptionfactory commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r680955581
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/serialization/jaxb/JAXBSerializer.java
##########
@@ -73,8 +79,12 @@ public void serialize(final int dataModelVersion, final T t, final OutputStream
try {
final Marshaller marshaller = jaxbContext.createMarshaller();
marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
- marshaller.marshal(t, out);
- } catch (JAXBException e) {
+
+ String className = clazz.getSimpleName();
+ String tagName = Character.toLowerCase(className.charAt(0)) + className.substring(1);
+
+ marshaller.marshal(new JAXBElement<>(new QName(tagName), clazz, t), out);
Review comment:
I would prefer not to have a try-catch with fall back in this case, because the reason would not be clear. Although having a direct reference to `VersionedProcessGroup` might not generally be preferable, it is a clear indicator of the reason for the workaround.
Use of reflection is simply introspecting the class using `Class.getAnnotation(XmlRootElement.class)` to determine whether the workaround is necessary. The getAnnotation() approach would be the most generic, and would avoid referencing VersionedProcessGroup, although it would be helpful to include a comment describing the reason for the workaround.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] kevdoran commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
kevdoran commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r650013517
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
Maybe it makes sense to just not include nifi-versioned-components in nifi-api, to avoid the transitive dependency issue? Do they have to be combined? If not, anyone who need the nifi-versioned-components data model can depend on that and deal with the transitive dependency issue.
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
Yeah, I am not aware of future plans for this data model. I just figured that of course every existing use of the versioned components data model must be OK with that transitive dependency, so why not just separate them from nifi-api... Of course that might not be the case forever, but given the motivation is code reuse, I'm just trying to avoid a solution that involves duplicating this data model to have one annotated version and one non-annotated.
The swagger annotations in Registry are not just used for REST API documentation. The swagger.json file that is generated from these annotations are used in three places:
- To generate the HTML REST API doc (could be done via Enunciate)
- By a self-hosted Swagger UI that is dynamically generated based on the contents of swagger.json
- In clients that use swagger-codegen to generate REST API client sdks from this spec. Currently this includes [nipyapi](https://github.com/Chaffelson/nipyapi), but I believe there was also talk of replacing much of the nifi-registry-web-ui data model with TypeScript generated from the REST API swagger spec.
I'm not opposed to having a separate DTO model that can support annotations and internal model of POJOs, and mappers/converters between them, if that is the only way to satisfy current and future needs.
--
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] [nifi] kevdoran commented on pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
kevdoran commented on pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#issuecomment-868563195
Yep, agree with @exceptionfactory and @markap14 regarding that dependency scope, and aside from that this looks good to me.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r650882924
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
I'll remove the nifi-api -> nifi-versioned-components dependency then.
When I open a new PR for Flow Analysis, I'll have nifi-framework-core depend on it as well.
As suggested, later when an extension needs the VersionedComponent classes, it needs to introduce this dependency as well.
It doesn't seem to be too hard to change later if we need it and now we can move forward.
--
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] [nifi] exceptionfactory commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r680952160
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/serialization/jaxb/JAXBSerializer.java
##########
@@ -88,9 +98,15 @@ public T deserialize(final InputStream input) throws SerializationException {
try {
// Consume the header bytes.
readDataModelVersion(input);
+
+ XMLStreamReader streamReader = XMLInputFactory.newInstance().createXMLStreamReader(input);
+
final Unmarshaller unmarshaller = jaxbContext.createUnmarshaller();
- return (T) unmarshaller.unmarshal(input);
- } catch (JAXBException e) {
+ JAXBElement<T> jaxbElement = unmarshaller.unmarshal(streamReader, clazz);
+ T deserializedObject = jaxbElement.getValue();
+
+ return deserializedObject;
Review comment:
I can see usefulness in a separate variable in some cases, particularly if it is the result of a complicated method. In this case, the value is a member variable of `JAXBElement`. IntelliJ flags this as redundant in the default configuration, which is not an absolute rule, but in this particular instance, having a separate variable does not seem to add much value for debugging.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] markap14 merged pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
markap14 merged pull request #5145:
URL: https://github.com/apache/nifi/pull/5145
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r650882924
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
I'll remove the nifi-api -> nifi-versioned-components dependency then.
When I open a new PR for Flow Analysis, I'll have nifi-framework-core depend on it instead.
As suggested, later when an extension needs the VersionedComponent classes, it needs to introduce this dependency as well.
It doesn't seem to be too hard to change later if we need it and now we can move forward.
--
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] [nifi] tpalfy commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r680970230
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/serialization/jaxb/JAXBSerializer.java
##########
@@ -88,9 +98,15 @@ public T deserialize(final InputStream input) throws SerializationException {
try {
// Consume the header bytes.
readDataModelVersion(input);
+
+ XMLStreamReader streamReader = XMLInputFactory.newInstance().createXMLStreamReader(input);
+
final Unmarshaller unmarshaller = jaxbContext.createUnmarshaller();
- return (T) unmarshaller.unmarshal(input);
- } catch (JAXBException e) {
+ JAXBElement<T> jaxbElement = unmarshaller.unmarshal(streamReader, clazz);
+ T deserializedObject = jaxbElement.getValue();
+
+ return deserializedObject;
Review comment:
Again, I find it useful to return a single variable.
During debug, one can just put a breakpoint at the return statement and see right away what that value is.
Also it's great that one can actually give a descriptive _name_ for the variable which can help understand the code better.
All in all I consider this a matter of preference and plan to leave it as it is unless I see more suggestions form others to do otherwise.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] tpalfy commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
tpalfy commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r680982282
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/serialization/jaxb/JAXBSerializer.java
##########
@@ -73,8 +79,12 @@ public void serialize(final int dataModelVersion, final T t, final OutputStream
try {
final Marshaller marshaller = jaxbContext.createMarshaller();
marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
- marshaller.marshal(t, out);
- } catch (JAXBException e) {
+
+ String className = clazz.getSimpleName();
+ String tagName = Character.toLowerCase(className.charAt(0)) + className.substring(1);
+
+ marshaller.marshal(new JAXBElement<>(new QName(tagName), clazz, t), out);
Review comment:
Referencing `VersionedProcessGroup` would not be a good indicator of the reason here in my opinion. The real problem is the `XmlRootElement` (or the lack thereof). It just so happens that at this point it's the `VersionedProcessGroup` that is missing it.
In other words, if we had an `if-else`, the reason why the `VersionedProcessGroup` is problematic wouldn't be clear at all. Also it's almost guaranteed that we would never recognise when this check becomes no longer necessary and as specific as it would be it'd become a piece of code that would be there for 'historical reasons'.
`clazz.getAnnotation(XmlRootElement.class)` brings in the `XmlRootElement` reference but I guess that's a reasonable compromise.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r649972869
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
Thanks for the reply @tpalfy.
Having an additional build dependency is not a problem, but having an additional runtime dependency is the concern. If the Swagger annotations did not have a runtime retention policy, then it seems like this approach could work. However, if the class loader needs to have access to the Swagger annotations at runtime, then this creates issues for extension components. Having a runtime dependency would require any extension components that access these classes to include the Swagger annotations dependency, which is a problem. As it stands, this impacts not just the nifi-framework bundle, but extension bundles as well, which might want to leverage the versioned components.
Unless I am missing something, it seems that we need to look at an approach that avoids introducing new transitive runtime dependencies into `nifi-api`.
Perhaps @markap14 can provide additional insight on the implications.
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
I was thinking along the same lines @kevdoran. I believe these changes will need to be part of the work necessary to implement the Flow Analysis capability, but if this PR avoids introducing `nifi-versioned-components` as a dependency in `nifi-api`, the issue with Swagger annotations could be addressed separately.
One potential solution discussed offline was moving back to [Enunciate](http://enunciate.webcohesion.com/) for generating REST API documentation from Java documentation comments, as opposed to the Swagger annotations. With the understanding that more work would be involved to evaluate that option, it could be handled in a separate PR and this one could move forward with changing the dependencies in `nifi-api` for now.
--
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] [nifi] exceptionfactory commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r649605589
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
Is there a specific reason this is marked as a `provided` dependency? This places a burden on referencing modules to include the dependency explicitly as a runtime dependency when needed.
##########
File path: nifi-api/pom.xml
##########
@@ -22,5 +22,13 @@
</parent>
<artifactId>nifi-api</artifactId>
<packaging>jar</packaging>
- <!-- This module should kept to having no dependencies -->
+
+ <!-- This module should kept to having no dependencies except nifi-versioned-components -->
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi-versioned-components</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </dependency>
+ </dependencies>
Review comment:
Is there a specific reason for introducing `nifi-versioned-components` as a dependency to `nifi-api` in this pull request? There do not appear to be any other changes to `nifi-api` in this pull request, so it seems like this change is not necessary right now. In addition, this introduces the transitive dependency on `swagger-annotations`.
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
Review comment:
This comment is confusing in light of the dependency below.
--
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] [nifi] exceptionfactory commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r649606487
##########
File path: nifi-api/pom.xml
##########
@@ -22,5 +22,13 @@
</parent>
<artifactId>nifi-api</artifactId>
<packaging>jar</packaging>
- <!-- This module should kept to having no dependencies -->
+
+ <!-- This module should kept to having no dependencies except nifi-versioned-components -->
+ <dependencies>
+ <dependency>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi-versioned-components</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </dependency>
+ </dependencies>
Review comment:
Is there a specific reason for introducing `nifi-versioned-components` as a dependency to `nifi-api` in this pull request? There do not appear to be any other changes to `nifi-api` in this pull request, so it seems like this change is not necessary right now. In addition, this introduces the transitive dependency on `swagger-annotations`.
--
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] [nifi] exceptionfactory commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r650976346
##########
File path: nifi-versioned-components/pom.xml
##########
@@ -0,0 +1,52 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ 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.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+ <modelVersion>4.0.0</modelVersion>
+ <parent>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
+ </parent>
+
+ <artifactId>nifi-versioned-components</artifactId>
+
+ <profiles>
+ <profile>
+ <id>nifi-versioned-components_jigsaw</id>
+ <activation>
+ <jdk>(1.8,)</jdk>
+ </activation>
+ <dependencies>
+ <dependency>
+ <groupId>javax.xml.bind</groupId>
+ <artifactId>jaxb-api</artifactId>
+ </dependency>
+ </dependencies>
+ </profile>
+ </profiles>
+
+ <!-- This module should kept to having no dependencies -->
+ <dependencies>
+ <dependency>
+ <groupId>io.swagger</groupId>
+ <artifactId>swagger-annotations</artifactId>
+ <version>1.5.16</version>
+ <scope>provided</scope>
Review comment:
Thanks for the reply @tpalfy, I understand the dependency change needed for Flow Analysis. It is helpful to get this refactor completed separately, so that the review of the Flow Analysis can be more focused. In either case, the main question comes to back to introducing the runtime dependency on `swagger-annotations` to `nifi-api`.
It sounds like avoiding the new runtime dependency in `nifi-api` would involve more work introducing a new model architecture that does not include the Swagger annotations.
--
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] [nifi] markap14 commented on pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
markap14 commented on pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#issuecomment-868562295
I reviewed the code and did some testing. Here are my findings:
- No no dependencies introduced into nifi root lib/ directory except for nifi-versioned-components-api.
- NiFi Registry still starts up and works as expected. Was able to push flows, pull flows, change versions from NiFi.
- Was able to push flows, pull flows, change versions when interacting with an older version of registry. Including flows that already existed and newly created/pushed flows.
- Can go to localhost:18080/nifi-registry-api/swagger/ui.html and am still able to see all of the appropriate documentation, including the full documentation for VersionedProcessGroup/VersionedProcessor, etc. that I got to from the "flows" section of the API.
I agree with @exceptionfactory that it makes sense to only include the swagger-annotations dependency where the nifi-registry-data-model module is used but otherwise, all looks good to me. I'm a +1 as soon as that gets updated.
Thanks for pulling all this together @tpalfy - I think this is a definitely a good move in the right direction. And thanks to @exceptionfactory for the thorough review & @kevdoran for helping with the review & providing insights!
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a change in pull request #5145: NIFI-8671 Create nifi-versioned-components module.
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on a change in pull request #5145:
URL: https://github.com/apache/nifi/pull/5145#discussion_r680163592
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/serialization/jaxb/JAXBSerializer.java
##########
@@ -88,9 +98,15 @@ public T deserialize(final InputStream input) throws SerializationException {
try {
// Consume the header bytes.
readDataModelVersion(input);
+
+ XMLStreamReader streamReader = XMLInputFactory.newInstance().createXMLStreamReader(input);
+
final Unmarshaller unmarshaller = jaxbContext.createUnmarshaller();
- return (T) unmarshaller.unmarshal(input);
- } catch (JAXBException e) {
+ JAXBElement<T> jaxbElement = unmarshaller.unmarshal(streamReader, clazz);
+ T deserializedObject = jaxbElement.getValue();
+
+ return deserializedObject;
Review comment:
This could be collapsed into a single line.
```suggestion
return jaxbElement.getValue();
```
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-data-model/pom.xml
##########
@@ -33,6 +33,11 @@
<groupId>javax.ws.rs</groupId>
<artifactId>javax.ws.rs-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.nifi</groupId>
+ <artifactId>nifi-api</artifactId>
+ <version>1.14.0-SNAPSHOT</version>
Review comment:
This should be updated to 1.15.0-SNAPSHOT to match the current main branch.
```suggestion
<version>1.15.0-SNAPSHOT</version>
```
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/serialization/jaxb/JAXBSerializer.java
##########
@@ -88,9 +98,15 @@ public T deserialize(final InputStream input) throws SerializationException {
try {
// Consume the header bytes.
readDataModelVersion(input);
+
+ XMLStreamReader streamReader = XMLInputFactory.newInstance().createXMLStreamReader(input);
Review comment:
Is it necessary to create an `XMLStreamReader` as opposed to wrapping the input in a `StreamSource`?
##########
File path: nifi-registry/nifi-registry-core/nifi-registry-framework/src/main/java/org/apache/nifi/registry/serialization/jaxb/JAXBSerializer.java
##########
@@ -73,8 +79,12 @@ public void serialize(final int dataModelVersion, final T t, final OutputStream
try {
final Marshaller marshaller = jaxbContext.createMarshaller();
marshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
- marshaller.marshal(t, out);
- } catch (JAXBException e) {
+
+ String className = clazz.getSimpleName();
+ String tagName = Character.toLowerCase(className.charAt(0)) + className.substring(1);
+
+ marshaller.marshal(new JAXBElement<>(new QName(tagName), clazz, t), out);
Review comment:
This approach seems like it should work for most cases given that current `XmlRootElement` annotations that specify a `name` attribute follow this pattern. However, what do you think about making this specific to `VersionedProcessGroup`? So if this class is an instance of `VersionedProcessGroup`, then use this approach, otherwise use the current `marshal()` call.
Another alternative could be using reflection to see whether the class has the `XmlRootElement` annotation.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org