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 support for JsonSubTypes #1336

Merged
merged 23 commits into from
Sep 25, 2023
Merged

feat: add support for JsonSubTypes #1336

merged 23 commits into from
Sep 25, 2023

Conversation

cromoteca
Copy link
Contributor

@cromoteca cromoteca commented Sep 22, 2023

Adds a union type for JsonSubTypes.

This is the supported configuration:

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY)
@JsonSubTypes({
    @JsonSubTypes.Type(value = UpFilter.class, name = "Up"),
    @JsonSubTypes.Type(value = DownFilter.class, name = "Down")}
)
public class Filter {
    public int dummy;
}

Closes #438
Closes #1255

@cromoteca cromoteca changed the title Feat/438/JsonSubTypes feat: add support for JsonSubTypes Sep 22, 2023
@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Patch coverage: 97.24% and project coverage change: +0.08% 🎉

Comparison is base (080a969) 96.18% compared to head (4b09c54) 96.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1336      +/-   ##
==========================================
+ Coverage   96.18%   96.27%   +0.08%     
==========================================
  Files          14       17       +3     
  Lines        1679     1824     +145     
  Branches      139      155      +16     
==========================================
+ Hits         1615     1756     +141     
- Misses         64       68       +4     
Flag Coverage Δ
unittests 96.27% <97.24%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...ypescript-plugin-subtypes/src/ModelFixProcessor.ts 95.00% <95.00%> (ø)
...typescript-plugin-subtypes/src/TypeFixProcessor.ts 96.15% <96.15%> (ø)
...ypescript-plugin-subtypes/src/SubTypesProcessor.ts 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Artur-
Copy link
Member

Artur- commented Sep 22, 2023

This is what I see from the react-grid-test module

WARN (tsgen) <BackbonePlugin>: Component has no properties:' dev.hilla.mappedtypes.PageableUnion'
WARN (tsgen) <BackbonePlugin>: Component has no properties:' dev.hilla.crud.filter.Filter'
WARN (tsgen) <BackbonePlugin>: Component has no properties:' dev.hilla.test.reactgrid.CompanyUnion'
WARN (tsgen) <BackbonePlugin>: Component has no properties:' dev.hilla.test.reactgrid.PersonUnion'
WARN (tsgen) <BackbonePlugin>: Component has no properties:' dev.hilla.mappedtypes.SortUnion'
WARN (tsgen) <BackbonePlugin>: Component has no properties:' dev.hilla.crud.filter.PropertyStringFilter$MatcherUnion'
WARN (tsgen) <BackbonePlugin>: Component has no properties:' dev.hilla.mappedtypes.OrderUnion'
WARN (tsgen) <BackbonePlugin>: Component has no properties:' org.springframework.data.domain.Sort$DirectionUnion'
WARN (tsgen) <BackbonePlugin>: Component has no properties:' org.springframework.data.domain.Sort$NullHandlingUnion'
WARN (tsgen) <ModelPlugin>: Component has no properties: dev.hilla.mappedtypes.PageableUnion
WARN (tsgen) <ModelPlugin>: Component has no properties: dev.hilla.crud.filter.Filter
ERROR (tsgen) <ModelPlugin>: The schema for a class component dev.hilla.crud.filter.FilterUnion is broken.
    type: "object"
    oneOf: [
      {
        "$ref": "#/components/schemas/dev.hilla.crud.filter.OrFilter"
      },
      {
        "$ref": "#/components/schemas/dev.hilla.crud.filter.AndFilter"
      },
      {
        "$ref": "#/components/schemas/dev.hilla.crud.filter.PropertyStringFilter"
      }
    ]
WARN (tsgen) <ModelPlugin>: Component has no properties: dev.hilla.test.reactgrid.CompanyUnion
WARN (tsgen) <ModelPlugin>: Component has no properties: dev.hilla.test.reactgrid.PersonUnion
WARN (tsgen) <ModelPlugin>: Component has no properties: dev.hilla.mappedtypes.SortUnion
WARN (tsgen) <ModelPlugin>: Component has no properties: dev.hilla.crud.filter.PropertyStringFilter$MatcherUnion
WARN (tsgen) <ModelPlugin>: Component has no properties: dev.hilla.mappedtypes.OrderUnion
WARN (tsgen) <ModelPlugin>: Component has no properties: org.springframework.data.domain.Sort$DirectionUnion
WARN (tsgen) <ModelPlugin>: Component has no properties: org.springframework.data.domain.Sort$NullHandlingUnion

and then it fails with

[ERROR] frontend/generated/dev/hilla/crud/filter/FilterUnionModel.ts(3,16): error TS2304: Cannot find name 'FilterUnionModel'.
% cat frontend/generated/dev/hilla/crud/filter/FilterUnionModel.ts 
import { _getPropertyModel as _getPropertyModel_1 } from "@hilla/form";
import type FilterUnion_1 from "./FilterUnion.js";
export default FilterUnionModel;

@cromoteca
Copy link
Contributor Author

You tried too fast ;)

@cromoteca cromoteca marked this pull request as draft September 22, 2023 13:42
@cromoteca cromoteca requested a review from Lodin September 22, 2023 14:24
@cromoteca cromoteca marked this pull request as ready for review September 22, 2023 14:25
@cromoteca cromoteca marked this pull request as draft September 25, 2023 09:25
@cromoteca cromoteca removed the request for review from Lodin September 25, 2023 09:25
@cromoteca cromoteca marked this pull request as ready for review September 25, 2023 11:24
@cromoteca cromoteca requested a review from Lodin September 25, 2023 11:24
Lodin
Lodin previously approved these changes Sep 25, 2023
Copy link
Contributor

@Lodin Lodin left a comment

Choose a reason for hiding this comment

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

LGTM

@Lodin
Copy link
Contributor

Lodin commented Sep 25, 2023

BTW, is it gonna be supported by EndpointInvoker runtime?

@cromoteca
Copy link
Contributor Author

BTW, is it gonna be supported by EndpointInvoker runtime?

What support should be added? You'll send normal types anyway, never the union. It doesn't exist in Java.

@Lodin
Copy link
Contributor

Lodin commented Sep 25, 2023

Yeah, I noticed that @type is not used by the model, so I was wondering about how it is supported on the Java end.

@cromoteca
Copy link
Contributor Author

Jackson will handle it to create an instance of the right type in Java, so it is not part of the object.

@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2023

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 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@cromoteca cromoteca requested a review from Lodin September 25, 2023 13:46
@cromoteca cromoteca enabled auto-merge (squash) September 25, 2023 13:49
@cromoteca cromoteca merged commit a266700 into main Sep 25, 2023
5 of 7 checks passed
@cromoteca cromoteca deleted the feat/438/JsonSubTypes branch September 25, 2023 14:47
taefi pushed a commit that referenced this pull request Sep 27, 2023
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Hilla 2.3.0.alpha6 and is also targeting the upcoming stable 2.3.0 version.

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.

Support @JsonSubType for method parameters Support polymorphic return types through @JsonSubTypes
5 participants