New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FUSETOOLS-2564 - WSDL 2 Camel Rest DSL Wizard #1138
Conversation
Can now run the wsdl2rest utility code through the wizard. Working on updating the wizard to improve that experience next. |
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.
very quick overview.
- I find it hard to know what you have to modify from original codebase to have ti working, if we can have the "core" functionality in a separate repo I think that it will really help for reviewing and understanding it
- I tried to put feedback only on few things that will surely stay here in the future and that can already be improved
@@ -0,0 +1,50 @@ | |||
package org.fusesource.ide.wsdl2rest.ui; |
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.
missing header
try { | ||
generate(new URL(page.getWsdlURL()), page.getOutputPathURL()); | ||
} catch (Exception e) { | ||
// do something with this |
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.
return false and log error ?
private String beanClass; | ||
|
||
public Wsdl2RestWizardPage(String pageName) { | ||
super(pageName); |
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.
what about calling this(pageName, null, null) to avoid duplicating the setMessage ?
} | ||
} | ||
} catch (JavaModelException e) { | ||
e.printStackTrace(); |
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.
don't forget to use logger instead of printStackTrace
@@ -0,0 +1,9 @@ | |||
package org.jboss.fuse.wsdl2rest; |
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.
missing header
Thanks for the feedback @apupier . I will add a readme file to help folks get some context on how to test this and will work through all of your suggestions. That said, this is merely a prototype branch and not meant to be the end-all-and-be-all, so until we figure out what we're doing with the core wsdl2rest code I am going to avoid splitting it out just yet. |
be145a5
to
d861439
Compare
7fbe6f1
to
170d8e0
Compare
@@ -0,0 +1,31 @@ | |||
# Copyright (c) 2017 Red Hat, Inc. |
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.
shouldn't the path be .../l10n/messages.properties?
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.
Heh. Oops!
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.
few pure code feedback
Bundle-ManifestVersion: 2 | ||
Bundle-Name: wsdl2rest UI plug-in | ||
Bundle-SymbolicName: org.fusesource.ide.wsdl2rest.ui;singleton:=true | ||
Bundle-Version: 10.1.0.qualifier |
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.
we are now on 10.2
<parent> | ||
<groupId>org.fusesource.ide.editor</groupId> | ||
<artifactId>plugins</artifactId> | ||
<version>10.1.0-SNAPSHOT</version> |
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.
we are now on 10.2
<dependency> | ||
<groupId>commons-validator</groupId> | ||
<artifactId>commons-validator</artifactId> | ||
<version>1.4.0</version> |
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.
Optional: latest version is 1.6.0 https://commons.apache.org/proper/commons-validator/ in case it is possible to upgrade, it might be a good idea to start with latest version.
I'm wondering if we shouldn't try to ask Jboss tools to provide an osgi version of it in the Target Platform
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.6.0 doesn't seem to be available in our maven repos currently, but this would be good to investigate
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.
they are using 1.6 version: https://mvnrepository.com/artifact/commons-validator/commons-validator/1.6
2 additional notes:
- I guess it needs to be updated in wsdl to rest first
- it would be even best if we can provide an orbit bundle to avoid embedding the lib in the plugin
<artifactId>maven-enforcer-plugin</artifactId> | ||
<executions> | ||
<execution> | ||
<id>ban-uberjars</id> |
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.
Do you know from where this rule comes from? it is from Jboss tools? if yes, it might give more importantce to have the commons-validator available in Target Platform.
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 was copied from the switchyard plug-ins (org.switchyard.tools.cxf was the model I used for the wsdl2rest code wrapper). Not sure we need it. Will investigate removing it when I start migrating to pulling wsdl2rest as a maven artifact instead of building a one-off copy of the code directly.
@@ -0,0 +1,50 @@ | |||
/******************************************************************************* | |||
* Copyright (c) 2015 Red Hat, Inc. |
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.
2017
}; | ||
} | ||
}; | ||
dialog.setInitialPattern("* "); //$NON-NLS-1$ |
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.
why '* ' and not '*'? (with a space after the asterisk)
} | ||
|
||
setControl(composite); | ||
setPageComplete(isPageComplete()); |
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.
indentation issue
} | ||
|
||
setControl(composite); | ||
setPageComplete(isPageComplete()); |
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.
indentation issue
ControlDecorationSupport.create(beanClassBinding, SWT.LEFT | SWT.TOP); | ||
|
||
// set initial values | ||
if (!Strings.isEmpty(getOptionsFromWizard().getDestinationJava())) { |
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.
what do you think to extract a little method such as:
public void initIfNotEmpty (Text textControl, String value) {
if (!Strings.isEmpty(value)) {
textControl.setText(value);
}
}
it will allow to write:
initIfNotEmpty(javaPathTextControl, getOptionsFromWizard().getDestinationJava());
initIfNotEmpty(camelPathTextControl, getOptionsFromWizard().getDestinationCamel());
initIfNotEmpty(targetAddressText, getOptionsFromWizard().getTargetServiceAddress());
initIfNotEmpty(beanClassText, getOptionsFromWizard().getBeanClassName());
Bundle-ManifestVersion: 2 | ||
Bundle-Name: Wsdl2rest | ||
Bundle-SymbolicName: org.fusesource.ide.wsdl2rest | ||
Bundle-Version: 10.1.0.qualifier |
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.
10.2
} | ||
|
||
private boolean resourceExists(String resource) { | ||
for (String str : this.resources) { |
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 think that resources.contains(resource); can be used
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.
We will be pulling this code in jarred form from maven, so I won't change it at this time, but a good spot.
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 created a PR upstream jboss-fuse/wsdl2rest#34
@@ -0,0 +1,128 @@ | |||
package org.jboss.fuse.wsdl2rest.impl; |
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.
missing header
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.
We will be pulling this code in jarred form from maven, so I won't change it at this time, but a good spot.
testPR |
testPR |
89c4e11
to
c5cd2ab
Compare
testPR |
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.
done a new pure code review. I will review functionally later.
@@ -0,0 +1,33 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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 project is not existing anymore, isn't it?
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.
see bfitzpat#25
This plug-in provides a wizard wrapper for the Wsdl2Rest code originally found here: [https://github.com/jboss-fuse/wsdl2rest](https://github.com/jboss-fuse/wsdl2rest) | ||
|
||
## Running the wizard | ||
NOTE: THIS DOES NOT WORK CURRENTLY, BUT DEMONSTRATES THE BASIC APPROACH. |
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.
don't forget to remove this line
On the "Specify Advanced Options for wsdl2rest Processing", click the "..." beside the Bean Class field and select the EchoServiceImpl class. | ||
Click Finish. | ||
|
||
Select the project and refresh it to see the generated code and new rest-camel-context.xml file. |
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.
shouldn't we say "the generated code and the new rest-camel-context.xml file."?
But I'm not the native speaker, just checking that it has not be forgotten (and if you can explain me what is the rule for that if i'm wrong it will help me to improve my English skill)
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 say "...to see the generated code and new rest-camel-context.xml file." The second "the" you included is implied because it's a simple list of two items. You could certainly include it, but it's not necessary.
<dependency> | ||
<groupId>org.apache.camel</groupId> | ||
<artifactId>camel-jackson</artifactId> | ||
<version>2.20.0</version> |
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.
currently the wsdl-to-rest project is based on Camel 2.18.0, does it have an impact?
Is it worthy to put 2.20.1 now that it is released?
|
||
The generated echo.Echo class will have many errors due to some missing dependencies. | ||
|
||
Add the following dependencies to your project pom.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.
Why do we need to add these dependencies? it is just because of the particular use case or it will be always?
If always, can you create a task to add it automatically please?
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.
|
||
<artifactId>org.fusesource.ide.wsdl2rest</artifactId> | ||
<packaging>eclipse-plugin</packaging> | ||
<name>JBoss Fuse Tooling :: Camel Editor :: Plugins :: Wsdl2Rest Implemenatation </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.
typo Implemenatation -> Implementation
<version.junit>4.11</version.junit> | ||
<version.wsdl4j>1.6.3</version.wsdl4j> | ||
<version.commons.lang>2.4</version.commons.lang> | ||
<version.wsdl2rest>0.7-SNAPSHOT</version.wsdl2rest> |
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 version will be changed when a (productized?) release will be available, 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.
Hope so, yes.
|
||
public class CamelCxfWsDocLitTest { | ||
|
||
// @Test |
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.
completely commented out test class, Should we remove it or uncomment it?
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.
Removed whole test plugin
|
||
public class CamelCxfWsRpcLitTest { | ||
|
||
// @Test |
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.
completely commented out test class, shoudl we remove it or uncomment it?
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.
Removed whole test plugin
Assert.assertEquals(5, methods.size()); | ||
} | ||
|
||
// @Test |
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.
is this commented out test relevant?
if yes, can you remove it please
If no, can you enable it please
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.
Removed whole test plugin
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
if needed Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Last commit adds better validation to the path fields (adding a warning if the path doesn't exist yet and can be created or an error if there's no way to create it, which shouldn't be the case) and adds the camel path if it needs to. I still think there should be a way to introspect the camel project and get the actual location (i.e. different between springboot, spring, and blueprint projects) and we could just use that to avoid having to create the folder. |
test is failing on CI:
|
maybe we can start by having a different path for springboot/spring/blueprint which are matching the templates that we are providing in Tooling? |
it is going to the default path registered on the OS. |
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Do we have a utility to discover if a project is Springboot vs. Spring vs. Blueprint? |
* removed duplicated "find resource" method and streamlined * added new tests to ensure Blueprint path is working * improved FuseProject class with better Blueprint support Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
…into FUSETOOLS-2564
In the process of work on making sure we're not overwriting files, it became clear that a bit more clean-up needed to happen.
|
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.
- there is still a case where the user can loose/override data without warning, it is if he clicks finish on the first page of the wizard as the warning is displayed only on the second page
- the warning is not displayed in my case:
-- I generated a first time
-- I'm trying to generate a second time on the same project with same wsdl
but maybe this modification that you show me in mattermost are not pushed yet?
@@ -13,7 +13,7 @@ Require-Bundle: org.eclipse.ui, | |||
org.eclipse.core.databinding, | |||
org.jboss.tools.foundation.core, | |||
org.jboss.tools.foundation.ui, | |||
org.fusesource.ide.foundation.core, | |||
org.fusesource.ide.foundation.core;visibility:=reexport, |
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.
better to always avoid to "reexport", if another plugin needs the dependencies, it should declare the dependencies in its MANIFEST too
Not exactly surely some code to reuse/factorize :-) |
Thanks for the hints as to springboot detection. And damn. Sorry about the FuseProject change. I'll see what I can figure out and get working again. |
* Finish is disabled if overwrite state could happen for Java files or for the Camel configuration * User now has the possibility to specify a specific camel file name for the generated config in the field * Also added support for Springboot projects so we get the path for generated camel files correct * Added new test for custom camel file in path Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
Signed-off-by: Brian Fitzpatrick <bfitzpat@redhat.com>
…into FUSETOOLS-2564
I believe I have fixed the issue with FuseProject - it doesn't fail locally now in those Red deer tests |
SonarQube analysis reported 9 issues Watch the comments in this conversation to review them. 7 extra issuesNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
I am not even able to build the branch. See https://gist.github.com/lhein/a72250fd3bf5d2e78cd870a9cc06baa6 |
also this PR is not rebased with latest master and still on version 10.x |
This one is now obsolete. We are going with #1362 |
Can close once we merge the other PR |
This is the first cut of the wsdl2rest code. The tests run after the jars are populated (mvn generate-sources) and the wizard attempts to run, but there's an issue with the wizard perform finish where it's not quite getting as far as the tests are. I suspect it has something to do with a classloader issue, but am still investigating.
VERY PRELIMINARY WORK