Skip to content

Add LocalDate/LocalDateTime/OffsetDateTime/ZonedDateTime/Instant support for query and filter #158

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

Merged
merged 24 commits into from
Aug 10, 2019

Conversation

SinaiNIN
Copy link
Contributor

@SinaiNIN SinaiNIN commented Aug 5, 2019

For now, the current library version does not support query and filter when we tried to query with where condition on LocalDate/LocalDateTime/OffsetDate/ZonedDateTime/Instant.

The error while query is

{
      "message": "Exception while fetching data : Unsupported field type class java.time.LocalDateTime",
}

The error on console log:

java.lang.NoSuchMethodException: java.time.LocalDateTime.(java.lang.Object)
at java.base/java.lang.Class.getConstructor0(Class.java:3350)
at java.base/java.lang.Class.getConstructor(Class.java:2152)

closes #157

@igdianov
Copy link
Collaborator

igdianov commented Aug 5, 2019

@SinaiNIN Great PR! Thank you for contributing. Seems like Travis is failing on the new test you have added because of missing Authors data in data.sql.

We also need more test coverage for new date/time datatypes added to predicate builder. It is best to cover them in a separate module similar to https://github.com/introproventures/graphql-jpa-query/blob/master/graphql-jpa-query-schema/src/test/java/com/introproventures/graphql/jpa/query/converter/GraphQLJpaConverterTests.java

Let me know if you have any questions or comments.

@igdianov
Copy link
Collaborator

igdianov commented Aug 5, 2019

@SinaiNIN If you add new test record to Books and Authors, it will break a lot of existing unit tests. It would be best to amend existing records with dateOfBirth and releaseDate and add new tests to cover LocalDate time support...

@codecov
Copy link

codecov bot commented Aug 6, 2019

Codecov Report

Merging #158 into master will decrease coverage by 1.6%.
The diff coverage is 49.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #158      +/-   ##
============================================
- Coverage     69.56%   67.95%   -1.61%     
- Complexity      450      495      +45     
============================================
  Files            33       33              
  Lines          2343     2578     +235     
  Branches        343      431      +88     
============================================
+ Hits           1630     1752     +122     
- Misses          576      646      +70     
- Partials        137      180      +43
Impacted Files Coverage Δ Complexity Δ
...ventures/graphql/jpa/query/schema/JavaScalars.java 45.51% <32.25%> (-1.16%) 5 <0> (ø)
...hql/jpa/query/schema/impl/JpaPredicateBuilder.java 53.52% <55.74%> (+1.53%) 102 <40> (+45) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15a4008...66e9c23. Read the comment docs.

@igdianov igdianov added the bug label Aug 6, 2019
@Test
public void queryLocalDateWithEqualTest() {
//given
String query = "query{\n" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SinaiNIN Can you, please, remove \n and \t from queries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@igdianov Noted, Thanks


}

@Test
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SinaiNIN Can you extract your tests into a separate a package with its own test class, data.sql and test model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can. It would be better.

Copy link
Collaborator

@igdianov igdianov left a comment

Choose a reason for hiding this comment

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

Lgtm

@igdianov igdianov merged commit a3f7877 into introproventures:master Aug 10, 2019
@MENGKONGSEAN
Copy link

Great thank b @SinaiNIN ❤️

@SinaiNIN SinaiNIN deleted the localDatefix branch October 26, 2021 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The query do not support with java.time.*
3 participants