Skip to content

Add SOAP CoDec (+ JAXB modifications)#786

Merged
velo merged 1 commit into
OpenFeign:masterfrom
pilak:feature-jaxb-soap
Nov 15, 2018
Merged

Add SOAP CoDec (+ JAXB modifications)#786
velo merged 1 commit into
OpenFeign:masterfrom
pilak:feature-jaxb-soap

Conversation

@pilak

@pilak pilak commented Sep 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Ready to merge
Thank you

@kdavisk6

kdavisk6 commented Sep 11, 2018

Copy link
Copy Markdown
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

@kdavisk6 kdavisk6 left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

pilak commented Sep 12, 2018

Copy link
Copy Markdown
Contributor Author

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

@pilak

pilak commented Sep 12, 2018

Copy link
Copy Markdown
Contributor Author

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

@pilak pilak force-pushed the feature-jaxb-soap branch from 7fb9d10 to 6294155 Compare October 14, 2018 17:37
@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

zjw2011 commented Oct 22, 2018

Copy link
Copy Markdown

support wsdl?

2 similar comments
@zjw2011

zjw2011 commented Oct 22, 2018

Copy link
Copy Markdown

support wsdl?

@zjw2011

zjw2011 commented Oct 22, 2018

Copy link
Copy Markdown

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

pilak commented Oct 27, 2018

Copy link
Copy Markdown
Contributor Author

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

pilak commented Oct 30, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

velo commented Oct 30, 2018

Copy link
Copy Markdown
Member

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
Comment thread soap/pom.xml
@pilak pilak force-pushed the feature-jaxb-soap branch from 013e097 to 2ceedb3 Compare November 1, 2018 18:09
@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

@velo velo left a comment

Copy link
Copy Markdown
Member

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

Comment thread 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" +

@velo velo Nov 1, 2018

Copy link
Copy Markdown
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

@pilak pilak Nov 4, 2018

Copy link
Copy Markdown
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.

Comment thread 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:

```

@velo velo Nov 1, 2018

Copy link
Copy Markdown
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

@pilak pilak Nov 4, 2018

Copy link
Copy Markdown
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 pilak force-pushed the feature-jaxb-soap branch from 2ceedb3 to 1cf8851 Compare November 4, 2018 17:31
@pilak

pilak commented Nov 15, 2018

Copy link
Copy Markdown
Contributor Author

Hi,
Any news about my PR ?
Regards

@velo

velo commented Nov 15, 2018

Copy link
Copy Markdown
Member

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

@velo velo merged commit 2bf7796 into OpenFeign:master Nov 15, 2018
@pilak

pilak commented Nov 17, 2018

Copy link
Copy Markdown
Contributor Author

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

@nipunarora

nipunarora commented Feb 11, 2020 •
edited
Loading

Copy link
Copy Markdown

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