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

[Bug][Java] defaultValues for date and date-time params #11536

Merged
merged 19 commits into from
Feb 12, 2022

Conversation

cachescrubber
Copy link
Contributor

@cachescrubber cachescrubber commented Feb 6, 2022

Support default values for Header and Cookie Params.

Fixes

PR checklist

  • [x ] Read the contribution guidelines.
  • [ x] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • [ x] Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • [ x] File the PR against the correct branch: master (5.3.0), 6.0.x
  • [ x] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@cachescrubber
Copy link
Contributor Author

@wing328 FYI. Who could help with the defaultValue date format issue?

@wing328
Copy link
Member

wing328 commented Feb 7, 2022

Is toDefaultValue what you're looking for?

I think you will need to override this method in the spring generator to address the issue above.

@cachescrubber
Copy link
Contributor Author

Is stumbled up this line:

return "OffsetDateTime.parse(\"" + String.format(Locale.ROOT, ((java.time.OffsetDateTime) schema.getDefault()).atZoneSameInstant(ZoneId.systemDefault()).toString(), "") + "\", java.time.format.DateTimeFormatter.ISO_ZONED_DATE_TIME.withZone(java.time.ZoneId.systemDefault()))";

What is the reasoning behind that?

@cachescrubber
Copy link
Contributor Author

@wing328 The result of toDefaultValue is ignore for all parameter type other than queryParams. Why is that? Any hint?

@cachescrubber
Copy link
Contributor Author

What is the reasoning behind that?

Answer to self: probably used to generate Model classes, not for Api method Parameter.

@cachescrubber
Copy link
Contributor Author

Relates to #8577

# Conflicts:
#	samples/client/petstore/java/okhttp-gson-nextgen/docs/FakeApi.md
#	samples/client/petstore/java/okhttp-gson-nextgen/src/main/java/org/openapitools/client/api/FakeApi.java
@cachescrubber
Copy link
Contributor Author

Slowly getting there. Need to find out why application/x-www-form-urlencoded are treated like model parameter.

@cachescrubber
Copy link
Contributor Author

@wing328 I added some more tests to cover this issue. BTW, I think it might be okay to move

org.openapitools.codegen.languages.AbstractJavaCodegen#toDefaultParameterValue

to DefaultCodegen. WDYT?

@cachescrubber cachescrubber changed the title [Bug][Java/Spring] defaultValues for header and cookie params [Bug][Java] defaultValues for date and date-time params Feb 9, 2022
@cachescrubber
Copy link
Contributor Author

Surprisingly there are no samples affected by the last changes. Should I add date params with defaults to the more widely used examples?

@cachescrubber
Copy link
Contributor Author

cachescrubber commented Feb 9, 2022

I did (local only). We would see stuff like the following (a groovy example)

    Date lastFeed = OffsetDateTime.parse("1973-12-19T11:39:57Z[UTC]", java.time.format.DateTimeFormatter.ISO_ZONED_DATE_TIME.withZone(java.time.ZoneId.systemDefault()))
    
    Date dateOfBirth = LocalDate.parse("2021-01-01")

so obviously the code in toDefaultValue() is not capable of supporting multiple Date libraries.

@cachescrubber
Copy link
Contributor Author

Ich enhanced toDefaultValue to only return a default for date or date-time when dateTimeLibrary is java8. This way we can safely remove legacy date apis without introducing more bugs and broken samples in the meantime.

@wing328 I'm happy with the PR, please review.

@wing328
Copy link
Member

wing328 commented Feb 12, 2022

@wing328 wing328 merged commit 9dfe8c6 into OpenAPITools:master Feb 12, 2022
@cachescrubber cachescrubber deleted the bugfix/spring_defaultValues branch February 12, 2022 10:01
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.

None yet

2 participants