Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add SOAP CoDec (+ JAXB modifications) #786

Merged
merged 1 commit into from
Nov 15, 2018
Merged

Add SOAP CoDec (+ JAXB modifications) #786

merged 1 commit into from
Nov 15, 2018

Conversation

pilak
Copy link
Contributor

@pilak pilak commented Sep 11, 2018 •
edited
Loading

Ready to merge
Thank you

@kdavisk6
Copy link
Member

Hot Take: this looks pretty good overall. We have a few JAXB related changes in flight. I think there will be conflicts going forward. In the meantime, can you look at the Travis build and correct any errors?

@kdavisk6 kdavisk6 mentioned this pull request Sep 12, 2018
Closed
kdavisk6
kdavisk6 reviewed Sep 12, 2018
View reviewed changes
Copy link
Member

@kdavisk6 kdavisk6 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments:

  • We prefer package-private visibility. Would it be possible to move the SOAPEncoder and SOAPDecoder classes up one level into feign.jaxb and make the members package-private? The reason for this is that we don't want to leak out our implementation details to users of the library.
  • Specifying a Charset should be optional, as it is for the rest of Feign. Can you refactor setting the default Charset to Util.UTF-8?
  • Can you add a test case that can decode a raw SOAP Envelope and verify that an encoded object produces a valid SOAP Envelope?

@kdavisk6 kdavisk6 added enhancement For recommending new capabilities waiting for feedback Issues waiting for a response from either to the author or other maintainers labels Sep 12, 2018
@pilak
Copy link
Contributor Author

pilak commented Sep 12, 2018

Ok no problem, I take that into account.
Thank you for your comments.

@pilak
Copy link
Contributor Author

pilak commented Sep 12, 2018

I will also manage for SOAP fault returns and check why StringWriter was originally buggy for me.

@pilak pilak changed the title [WIP] Add SOAP encoder/decoder and update jaxb [WIP] Adding SOAP CoDec (+ JAXB modifications) Oct 14, 2018
@pilak pilak changed the title [WIP] Adding SOAP CoDec (+ JAXB modifications) WIP Adding SOAP CoDec (+ JAXB modifications) Oct 14, 2018
@zjw2011
Copy link

zjw2011 commented Oct 22, 2018

support wsdl?

2 similar comments
@zjw2011
Copy link

zjw2011 commented Oct 22, 2018

support wsdl?

@zjw2011
Copy link

zjw2011 commented Oct 22, 2018

support wsdl?

@pilak pilak force-pushed the feature-jaxb-soap branch 4 times, most recently from a5d7cd9 to 640d15c Compare October 27, 2018 22:02
@pilak
Copy link
Contributor Author

pilak commented Oct 27, 2018

support wsdl?

You'll have to implement your own JAXB classes generation. Afterwhat the Feign API client will be capable of marshall/unmarshall your types. It does not handle WSDL parsing...

@pilak pilak force-pushed the feature-jaxb-soap branch 5 times, most recently from e6101c5 to 013e097 Compare October 30, 2018 12:36
@pilak pilak changed the title WIP Adding SOAP CoDec (+ JAXB modifications) Adding SOAP CoDec (+ JAXB modifications) Oct 30, 2018
@pilak
Copy link
Contributor Author

pilak commented Oct 30, 2018 •
edited
Loading

PR is unwip, but don't know why Travis publish fails on jdk11 ...?

@pilak pilak changed the title Adding SOAP CoDec (+ JAXB modifications) Add SOAP CoDec (+ JAXB modifications) Oct 30, 2018
@velo
Copy link
Member

velo commented Oct 30, 2018

Seems soap/pom.xml is being changed when the build runs.

I would recommend rebasing on feign/master, mvn clean install locally (that will re-format the poms if needed) and check if you can reproduce.

velo
velo reviewed Oct 30, 2018
View reviewed changes
soap/pom.xml Show resolved Hide resolved
@velo velo added ready to merge Will be merged if no other member ask for changes and removed waiting for feedback Issues waiting for a response from either to the author or other maintainers labels Nov 1, 2018
velo
velo approved these changes Nov 1, 2018
View reviewed changes
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few comments, I think it's good, but would love to see this big XML entries on .xml files

soap/src/test/java/feign/soap/SOAPFaultDecoderTest.java Outdated

public class SOAPFaultDecoderTest {

private static final String SOAP_1_2_FAULT = "<?xml version=\"1.0\"?>\n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice (for maintenance) to move this to files inside src/test/resources/samples

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Velo,
I externalized the SOAP Envelop in corresponding XML files.

soap/README.md Outdated

Because a SOAP Fault can be returned as well with a 200 http code than a 4xx or 5xx HTTP error code (depending on the used API), you may also use `SOAPErrorDecoder` in your API configuration, in order to be able to catch `SOAPFaultException` in case of SOAP Fault. Add it, like below:

```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addd java after ```java

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@velo velo added waiting for feedback Issues waiting for a response from either to the author or other maintainers and removed ready to merge Will be merged if no other member ask for changes labels Nov 4, 2018
@pilak
Copy link
Contributor Author

pilak commented Nov 15, 2018

Hi,
Any news about my PR ?
Regards

@velo
Copy link
Member

velo commented Nov 15, 2018

Yeah, it's good to go, sorry for the delay

@velo velo merged commit 2bf7796 into OpenFeign:master Nov 15, 2018
@pilak
Copy link
Contributor Author

pilak commented Nov 17, 2018

No problems, I had the source code anyway ;-)
Thank you

@nipunarora
Copy link

nipunarora commented Feb 11, 2020 •
edited
Loading

hi @pilak is there an example of how to make this work with WSDL? I have an existing WSDL based webServiceClient template that I would like to move to feign. I'd really appreciate any assistance on to how to use it with WSDL.

velo pushed a commit that referenced this pull request Oct 7, 2024
@pilak @velo
Adding SOAP CoDec (+ JAXB modifications) (#786)
73d8165
velo pushed a commit that referenced this pull request Oct 8, 2024
@pilak @velo
Adding SOAP CoDec (+ JAXB modifications) (#786)
5ab386e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdavisk6 kdavisk6 kdavisk6 left review comments

@velo velo velo approved these changes

Assignees
No one assigned
Labels
enhancement For recommending new capabilities waiting for feedback Issues waiting for a response from either to the author or other maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pilak @kdavisk6 @zjw2011 @velo @nipunarora