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.

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

Initial implementation of WSDL to Ballerina tool #14

Merged
merged 11 commits into from
Sep 23, 2024

Conversation

AzeemMuzammil
Copy link
Member

Purpose

$title. ballerina-platform/ballerina-library#6717

Goals

Describe the solutions that this feature/fix will introduce to resolve the problems described above

Approach

Describe how you are implementing the solutions. Include an animated GIF or screenshot if the change affects the UI (email documentation@wso2.com to review all UI text). Include a link to a Markdown file or Google doc if the feature write-up is too long to paste here.

User stories

Summary of user stories addressed by this change>

Release note

Brief description of the new feature or bug fix as it will appear in the release notes

Documentation

Link(s) to product documentation that addresses the changes of this PR. If no doc impact, enter “N/A” plus brief explanation of why there’s no doc impact

Training

Link to the PR for changes to the training content in https://github.com/wso2/WSO2-Training, if applicable

Certification

Type “Sent” when you have provided new/updated certification questions, plus four answers for each question (correct answer highlighted in bold), based on this change. Certification questions/answers should be sent to certification@wso2.com and NOT pasted in this PR. If there is no impact on certification exams, type “N/A” and explain why.

Marketing

Link to drafts of marketing content that will describe and promote this feature, including product page changes, technical articles, blog posts, videos, etc., if applicable

Automation tests

  • Unit tests

    Code coverage information

  • Integration tests

    Details about the test cases and coverage

Security checks

Samples

Provide high-level details about the samples related to this feature

Related PRs

List any other related PRs

Migrations (if applicable)

Describe migration steps and platforms on which migration has been tested

Test environment

List all JDK versions, operating systems, databases, and browser/versions on which this feature/fix was tested

Learning

Describe the research phase and any blog posts, patterns, libraries, or add-ons you used to solve the problem.

@AzeemMuzammil AzeemMuzammil marked this pull request as draft July 10, 2024 07:58
.gitignore Outdated Show resolved Hide resolved
@ayeshLK
Copy link
Member

ayeshLK commented Jul 10, 2024

@AzeemMuzammil why have we committed the .idea directory. AFAIK this is something we should omit.

@ayeshLK
Copy link
Member

ayeshLK commented Jul 10, 2024

@AzeemMuzammil any reason to commit dependency JARs (in the module-ballerina-wsdl/resources directory) ? I think we can download them into a lib folder during the build time, rather than committing them to repo.

List<String> processedOpOutputNames =
processedOperations.stream().map(op -> op.getOperationOutput().getName()).toList();
if (processedOpOutputNames.contains(wsdlOperation.getOperationOutput().getName())) {
return new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return new ArrayList<>();
return Collections.emptyList();

Comment on lines 95 to 98
for (WSDLPart outputPart : outputParts) {
List<Field> outputFields = partProcessor.generateFields(outputPart);
fields.addAll(outputFields);
}
Copy link
Member

Choose a reason for hiding this comment

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

Shall we check whether we can do this ?

Suggested change
for (WSDLPart outputPart : outputParts) {
List<Field> outputFields = partProcessor.generateFields(outputPart);
fields.addAll(outputFields);
}
outputParts.map(part -> partProcessor.generateFields(outputPart)).forEach(outputFields -> fields.addAll(outputFields));

Comment on lines 40 to 42
public void initializeSchemas(Map<String, XmlSchema> targetNSToSchema) {
this.targetNSToSchema = targetNSToSchema;
}
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this function to initialize the SchemeManager right ? We can do this within the constructor itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a singleton, Constructor is private

Comment on lines 131 to 136
private WSDLService getSOAPService(Definition wsdlDefinition) {
@SuppressWarnings("unchecked")
Collection<Service> services = wsdlDefinition.getAllServices().values();
for (Service service : services) {
@SuppressWarnings("unchecked")
Collection<Port> ports = service.getPorts().values();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private WSDLService getSOAPService(Definition wsdlDefinition) {
@SuppressWarnings("unchecked")
Collection<Service> services = wsdlDefinition.getAllServices().values();
for (Service service : services) {
@SuppressWarnings("unchecked")
Collection<Port> ports = service.getPorts().values();
@SuppressWarnings("unchecked")
private WSDLService getSOAPService(Definition wsdlDefinition) {
Collection<Service> services = wsdlDefinition.getAllServices().values();
for (Service service : services) {
Collection<Port> ports = service.getPorts().values();

for (Port port : ports) {
List<?> extensions = port.getExtensibilityElements();
for (Object extension : extensions) {
if (extension instanceof SOAPAddress || extension instanceof SOAP12Address) {
Copy link
Member

Choose a reason for hiding this comment

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

I would rather decouple this condition into if and if-else block. That would simplify the mental mapping and improve the readability. And it would allow us to use Java instanceOf pattern-matching [1] and remove the redundant casting.

[1] - https://www.baeldung.com/java-pattern-matching-instanceof

}
}
}
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to return null here ? IMO null should be avoided and use Optionals instead. I think this answer [1] cover the why we should use optionals in a high-level

[1] - https://softwareengineering.stackexchange.com/questions/309134/why-is-using-an-optional-preferential-to-null-checking-the-variable

.idea/.gitignore Outdated Show resolved Hide resolved
@@ -0,0 +1,68 @@
///*
// * Copyright (c) 2022, WSO2 Inc. (http://www.wso2.org) All Rights Reserved.

Choose a reason for hiding this comment

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

Licence should be updated

@AzeemMuzammil AzeemMuzammil marked this pull request as ready for review July 16, 2024 08:39

@Override
public void printLongDesc(StringBuilder out) {

Choose a reason for hiding this comment

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

Append your help text here to the out string builder. The CLI will print it from bal help wsdl


@Override
public void execute() {
if (inputPath == null || inputPath.isBlank()) {

Choose a reason for hiding this comment

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

Let's add a new --help flag. For this flag and for no options/ args, we can print the help command.

@CommandLine.Option(names = {"-i", "--input"}, description = "Relative path to the WSDL file")
private String inputPath;

@CommandLine.Option(names = {"--operations"}, description = "Comma-separated operation names to generate")

Choose a reason for hiding this comment

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

See if we can take operations into an array using the Picocli utilities itself.

* @since 0.1.0
*/
public class Messages {
public static final String MISSING_WSDL_PATH = "Error: Missing input WSDL file path. " +

Choose a reason for hiding this comment

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

Let's use a property file here

@@ -0,0 +1,20 @@
// Copyright (c) 2024 WSO2 Inc. (http://www.wso2.org).
Copy link

@gayaldassanayake gayaldassanayake Jul 16, 2024

Choose a reason for hiding this comment

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

We don't need any Ballerina code inside our package. We can have empty projects when we do a bal pack for tools. So let's remove this

@@ -0,0 +1 @@
Ballerina WSDL Tools

Choose a reason for hiding this comment

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

Let's have a proper Module.md that explains what this tool does. Although currently we don't display tool information in the Ballerina central, soon we will. The tool Module.md will be used to show the tool information (similar to what we do with packages)


[ballerina]
dependencies-toml-version = "2"
distribution-version = "2201.9.1"

Choose a reason for hiding this comment

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

Let's use 2201.9.2 since it is published now

Comment on lines +93 to +94
//publishToMavenLocal.dependsOn build
//publish.dependsOn build

Choose a reason for hiding this comment

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

If we don't need this dependency, can remove

*
* @since 0.1.0
*/
public class XsdToRecordGenerator {

Choose a reason for hiding this comment

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

Are we generating the closed record with this?

@Nuvindu Nuvindu changed the base branch from main to initial-version September 23, 2024 04:25
@Nuvindu Nuvindu merged commit 9df48a0 into ballerina-platform:initial-version Sep 23, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants