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

feat: add un/marshal JSON preset for Java #596

Merged
merged 15 commits into from
Feb 13, 2022

Conversation

ritik307
Copy link
Contributor

Description

  • Added un/marshalling in Java inside src/generators/java/presets/CommonPreset.ts

Related issue(s)

Fixed #488

@ritik307
Copy link
Contributor Author

hi @jonaslagoni, Kindly look into my PR and let me know if I am heading in the right direction or not. 🙂

@ritik307
Copy link
Contributor Author

@jonaslagoni I am currently facing errors while writing a test for un/marshalling in Java and not able to figure out what the error kindly look into them

@jonaslagoni
Copy link
Member

Definitely in the right direction 👍 Just keep iterating 🙂

@ritik307
Copy link
Contributor Author

ritik307 commented Feb 1, 2022

@jonaslagoni I have implemented the changes you have mentioned. Kindly look at them and let me know if any more changes are required. :)

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Have a couple of remarks 🙂

Remember your code smells from sonarcloud 🙂

src/generators/java/presets/CommonPreset.ts Outdated Show resolved Hide resolved
src/generators/java/presets/CommonPreset.ts Outdated Show resolved Hide resolved
src/generators/java/presets/CommonPreset.ts Outdated Show resolved Hide resolved
src/generators/java/presets/CommonPreset.ts Show resolved Hide resolved
@ritik307 ritik307 marked this pull request as ready for review February 4, 2022 14:02
@coveralls
Copy link

coveralls commented Feb 4, 2022

Pull Request Test Coverage Report for Build 1835217589

  • 32 of 34 (94.12%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 93.452%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/generators/java/presets/CommonPreset.ts 32 34 94.12%
Totals Coverage Status
Change from base Build 1835216233: -0.09%
Covered Lines: 2490
Relevant Lines: 2538

💛 - Coveralls

@ritik307
Copy link
Contributor Author

ritik307 commented Feb 4, 2022

@jonaslagoni everything is working now 😊. I ran npm run lint:fix and it was getting warning security/detect-object-injection. So as of now, I have added a command to skip this warning line as adding parseInt(prop,10) isn't helping and generating more errors.

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Small changes requested, looking good! 💪

We need to enlighten the user of the new preset, so they can start using it!

We need to adapt our Java language documentation: https://github.com/asyncapi/modelina/blob/master/docs/languages/Java.md

Here you could add something along the lines of:

## Include JSON marshaling and unmarshaling methods
Sometimes you just want to convert your class to JSON without the use of annotations such as Jackson.

Check out this [example for a live demonstration](https://github.com/asyncapi/modelina/blob/master/examples/java-generate-marshal).

External dependencies
- Requires [org.json package](https://search.maven.org/artifact/org.json/json/20211205/bundle) to work

Next, we need to create an example to show exactly how a user can enable this option (as well as always ensuring it works). We do that by adding the following example: https://github.com/asyncapi/modelina/blob/master/examples/java-generate-marshal

You can copy this template for a good start: https://github.com/asyncapi/modelina/tree/master/examples/TEMPLATE

Read more about adding examples here: https://github.com/asyncapi/modelina/blob/master/docs/contributing.md#adding-examples

Let me know if you have any questions!

Comment on lines 133 to 134
// eslint-disable-next-line
const getType = `jsonObject.get${properties[prop].type?.toString().charAt(0).toUpperCase()}${properties[prop].type?.toString().slice(1)}`;
Copy link
Member

@jonaslagoni jonaslagoni Feb 7, 2022

Choose a reason for hiding this comment

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

I am assuming these lines are the problems to eslint complaining?

Suggested change
// eslint-disable-next-line
const getType = `jsonObject.get${properties[prop].type?.toString().charAt(0).toUpperCase()}${properties[prop].type?.toString().slice(1)}`;
const getType = `jsonObject.get${FormatHelpers.upperFirst(properties[prop].type?.toString())}`

Was it the optional chaining of properties[prop].type?, or the access of properties[prop]?

You could try the following:

const propModel = properties[String(prop)];
if(propModel.type === 'undefined') {
  Logger.error(`Could not render unmarshal for property ${prop}`);
  return; 
}

src/generators/java/presets/CommonPreset.ts Outdated Show resolved Hide resolved
@ritik307
Copy link
Contributor Author

ritik307 commented Feb 7, 2022

hi @jonaslagoni , I have made changes but I am facing an error kindly look at them.
ss1

@jonaslagoni
Copy link
Member

hi @jonaslagoni , I have made changes but I am facing an error kindly look at them. ss1

That is indeed weird. Have you tried printing the content or debugging the test to see what is actually inside classModel.dependencies?

Is it just some hidden character not matching or?

@ritik307
Copy link
Contributor Author

ritik307 commented Feb 7, 2022

@jonaslagoni This error is actually occurring because I changed the code from

const shouldContainMarshal = options.marshal === undefined || options.marshal === true;

to

const shouldContainMarshal = options.marshal === true;

But if I remove the following imports the code works fine I think it is not able to process multiple imports.

expect(classModel.dependencies.includes('import java.util.stream;')).toEqual(true);
expect(classModel.dependencies.includes('import org.json.JSONObject;')).toEqual(true);

@jonaslagoni
Copy link
Member

jonaslagoni commented Feb 7, 2022

But if I remove the following imports the code works fine I think it is not able to process multiple imports.

It definitely should be 😄

Do you mind trying to print out what is inside the array console.log(classModel.dependencies)? 🙂

@jonaslagoni
Copy link
Member

Here we are testing two dependencies:

const expectedDependencies = ['import java.util.Map;', 'import com.fasterxml.jackson.annotation.*;'];

@ritik307
Copy link
Contributor Author

ritik307 commented Feb 7, 2022

Do you mind trying to print out what is inside the array console.log(classModel.dependencies)?
@jonaslagoni Here is the screenshot
ss1

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Small comments, nearly there, looking great 💪

docs/languages/Java.md Outdated Show resolved Hide resolved
@ritik307
Copy link
Contributor Author

@jonaslagoni Added example for marshalling. Kindly review :)

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

LGTM, awesome @ritik307 🎉

@jonaslagoni
Copy link
Member

/rtm

@jonaslagoni jonaslagoni changed the title feat: un/marshal in java feat: add un/marshal JSON preset for Java Feb 12, 2022
@sonarcloud
Copy link

sonarcloud bot commented Feb 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jonaslagoni jonaslagoni merged commit afe46b1 into asyncapi:master Feb 13, 2022
@jonaslagoni
Copy link
Member

@all-contributors please add @ritik307 for code, test, example

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.48.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@allcontributors
Copy link
Contributor

@jonaslagoni

I've put up a pull request to add @ritik307! 🎉

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0-next.23 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add helper functions to un/marshal data models for Java
4 participants