-
Notifications
You must be signed in to change notification settings - Fork 62
[OODT-1015] Workflow Manager V2 JAX-RS REST APIs #101
Conversation
webapp/wmservices/pom.xml
Outdated
</dependency> | ||
</dependencies> | ||
</project> | ||
<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file seems to be reformatted with tabs replaced with spaces and LF line endings replaced with CRLF. I reverted those and created the following pom. Please check that and update the PR accordingly.
<?xml version='1.0' encoding='UTF-8'?>
<!-- Licensed to the Apache Software Foundation (ASF) under one or more contributor
license agreements. See the NOTICE.txt 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/maven-v4_0_0.xsd">
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.apache.oodt</groupId>
<artifactId>oodt-core</artifactId>
<version>1.9-SNAPSHOT</version>
<relativePath>../../core/pom.xml</relativePath>
</parent>
<artifactId>workflow-services</artifactId>
<packaging>war</packaging>
<name>CAS Workflow REST Services</name>
<description>
A set of REST-ful services for the Workflow Manager.
</description>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<executions>
<execution>
<id>make-a-jar</id>
<phase>compile</phase>
<goals>
<goal>jar</goal>
</goals>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-install-plugin</artifactId>
<executions>
<execution>
<phase>install</phase>
<goals>
<goal>install-file</goal>
</goals>
<configuration>
<packaging>jar</packaging>
<artifactId>${project.artifactId}</artifactId>
<groupId>${project.groupId}</groupId>
<version>${project.version}</version>
<file>
${project.build.directory}/${project.artifactId}-${project.version}.jar
</file>
</configuration>
</execution>
</executions>
</plugin>
</plugins>
</build>
<dependencies>
<dependency>
<groupId>org.apache.oodt</groupId>
<artifactId>cas-workflow</artifactId>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>javax.servlet</groupId>
<artifactId>servlet-api</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.apache.cxf</groupId>
<artifactId>cxf-rt-frontend-jaxrs</artifactId>
</dependency>
<dependency>
<groupId>org.apache.cxf</groupId>
<artifactId>cxf-rt-rs-extension-providers</artifactId>
</dependency>
<dependency>
<groupId>org.apache.cxf</groupId>
<artifactId>cxf-rt-transports-local</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.cxf</groupId>
<artifactId>cxf-rt-transports-http-jetty</artifactId>
</dependency>
<dependency>
<groupId>org.codehaus.jettison</groupId>
<artifactId>jettison</artifactId>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
</project>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an inquiry. What's the difference between LF and CRLF. Both are line endings, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRLF (Carriage return line feed \r\n) is used by windows operating systems. LF (\n) is used by unix mostly. Windows will usually replace LF with CRLF when cloning repositories. This will cause git to consider the whole file as changed. In this file, I noticed that line endings have changed from LF to CRLF in your version. In future, if we want to see who changed a specific line, it will be you who will get annotated due to this, not the actual author.
*/ | ||
@XmlRootElement(name = "metadata") | ||
@XmlAccessorType(XmlAccessType.NONE) | ||
public class MetadataResource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this file taken from some other location? Like file manager services? If yes, we have some code duplication to address (we can do that later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I took it from FM resources. Same functionality. What is the code duplication process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually we need to have a common module to hold shared resource classes and both FM and WM should use it. We can do that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Most of the exceptions, resources and exception mappers are same for all the web apps. We can port those to common module. say "webapp-common"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Something like that.
WorkflowInstance workflowInstanceById = wmclient.getWorkflowInstanceById(workflowInstId); | ||
WorkflowInstanceResource workflowResource = | ||
new WorkflowInstanceResource(workflowInstanceById); | ||
logger.debug("WorkFlowInstance ID : " + workflowInstId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In SLF4J, we can write this as logger.debug("WorkFlowInstance ID : {}", workflowInstId);
. No need to concatenate manually.
@Path("workflowInst") | ||
@Produces({MediaType.APPLICATION_XML, MediaType.APPLICATION_JSON}) | ||
public WorkflowInstanceResource getWorkflowInstanceById( | ||
@QueryParam("workflowInstId") String workflowInstId) throws WebApplicationException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, query param workflowInstId
can be null since it is coming from the query params. Is that a valid scenario? If not, we should move that to a path param right?
webapp/wmservices/src/main/java/org/apache/oodt/cas/wmservices/services/WMJaxrsServiceV2.java
Show resolved
Hide resolved
@@ -10,12 +10,12 @@ | |||
OR CONDITIONS OF ANY KIND, either express or implied. See the License for | |||
the specific language governing permissions and limitations under the License. --> | |||
|
|||
<Context path="/cas-wm-services" reloadable="true" crossContext="true"> | |||
<Context crossContext="true" docBase="/path/to/cas-workflow-services.war" reloadable="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like formatting only changes. docBase
here can be left unchanged. See tomcat documentation on that. Is there any specific reason for the docBase
change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used docBase to locate the WAR file in tomcat server. It's just as the FM web app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That means, is this change necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this makes sense to a third-party developer to understand the usage of context.xml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. The changed version is missing the path attribute right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. path is replaced by docBase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In tomcat, AFAIK, path is the URL suffix while docBase is the path to war. I think we should need both?
<servlet-class> | ||
org.apache.oodt.cas.wmservices.servlets.WmServicesServlet | ||
</servlet-class> | ||
<servlet-name>WmServicesServlet</servlet-name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an inquiry. This was the old servlet definition right? Does that need any changes like this? I have removed some non-related formatting below.
<?xml version="1.0" encoding="UTF-8"?>
<!--
Licensed to the Apache Software Foundation (ASF) under one or more contributor
license agreements. See the NOTICE.txt 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.
-->
<web-app version="2.5"
xmlns="http://java.sun.com/xml/ns/javaee"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://java.sun.com/xml/ns/javaee
http://java.sun.com/xml/ns/javaee/web-app_2_5.xsd">
<display-name>CAS Workflow Manager Services</display-name>
<filter>
<filter-class>org.apache.oodt.cas.wmservices.filters.CORSFilter</filter-class>
<filter-name>CorsFilter</filter-name>
</filter>
<filter-mapping>
<filter-name>CorsFilter</filter-name>
<url-pattern>/*</url-pattern>
</filter-mapping>
<servlet>
<servlet-name>WmServicesServlet</servlet-name>
<servlet-class>
org.apache.oodt.cas.wmservices.servlets.WmServicesServlet
</servlet-class>
<init-param>
<param-name>jaxrs.serviceClasses</param-name>
<param-value>
org.apache.oodt.cas.wmservices.resources.WorkflowResource
</param-value>
</init-param>
<init-param>
<param-name>jaxrs.scope</param-name>
<param-value>prototype</param-value>
</init-param>
<load-on-startup>1</load-on-startup>
</servlet>
<servlet>
<init-param>
<param-name>jaxrs.serviceClasses</param-name>
<param-value>
org.apache.oodt.cas.wmservices.services.WMJaxrsServiceV2
</param-value>
</init-param>
<init-param>
<param-name>jaxrs.providers</param-name>
<param-value>
org.apache.oodt.cas.wmservices.exceptionmappers.NotFoundExceptionMapper,
org.apache.oodt.cas.wmservices.exceptionmappers.BadRequestExceptionMapper,
org.apache.oodt.cas.wmservices.exceptionmappers.InternalServerErrorExceptionMapper,
org.apache.oodt.cas.wmservices.exceptionmappers.CasWorkflowExceptionMapper,
</param-value>
</init-param>
<init-param>
<param-name>jaxrs.extensions</param-name>
<param-value>
file=application/octet-stream
json=application/json
rdf=application/rdf+xml
rss=application/rss+xml
xml=application/xml
zip=application/zip
</param-value>
</init-param>
<init-param>
<param-name>jaxrs.scope</param-name>
<param-value>prototype</param-value>
</init-param>
<load-on-startup>1</load-on-startup>
<servlet-class>
org.apache.oodt.cas.wmservices.servlets.WmServicesServlet
</servlet-class>
<servlet-name>WmServicesServletV2</servlet-name>
</servlet>
<!-- CORS Header Filtering -->
<servlet-mapping>
<servlet-name>WmServicesServletV2</servlet-name>
<url-pattern>/jaxrs/v2/*</url-pattern>
</servlet-mapping>
<servlet-mapping>
<servlet-name>WmServicesServlet</servlet-name>
<url-pattern>/service/*</url-pattern>
</servlet-mapping>
</web-app>
<!-- CORS Header Filtering --> | ||
<servlet-mapping> | ||
<servlet-name>WmServicesServletV2</servlet-name> | ||
<url-pattern>/jaxrs/v2/*</url-pattern> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename jaxrs/v2
to a meaningful name like wmservice/v2
?
@@ -90,13 +90,13 @@ public AvroRpcWorkflowManager(int port){ | |||
throw new IllegalStateException("Null engine"); | |||
} | |||
|
|||
URL workflowManagerUrl = safeGetUrlFromString("http://" + getHostname() + ":" + port); | |||
URL workflowManagerUrl = safeGetUrlFromString("http://localhost" + ":" + port); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be changed
if(workflowManagerUrl == null){ | ||
throw new IllegalStateException("Null workflow manager URL"); | ||
} | ||
|
||
logger.debug("Setting workflow engine url: {}", workflowManagerUrl.toString()); | ||
engine.setWorkflowManagerUrl(safeGetUrlFromString("http://" + getHostname() + ":" + port)); | ||
engine.setWorkflowManagerUrl(safeGetUrlFromString("http://localhost" + ":" + port)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be changed.
@NGimhana I have added some review comments above. Can you check? |
@IMS94 I did changes as you suggested. Extremely apologise for the code formatting issues. Accidently I have changed google style to another one. |
@NGimhana Thanks for the PR. Merged. |
This PR address JIRA Issue OODT-1015. PR contains the implementation of the next version(V2) of JAXRS REST APIs for OODT-Workflow Manager. These APIs satisfies API characteristics like providing back-compatibility, allowing CORS headers, exception handling with proper payloads. (endpoint pattern: /jaxrs/v2/*)