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

inherit the default behaviour of spring for internal object mapper #8051

Merged
merged 11 commits into from
Apr 16, 2020

Conversation

haijian-vaadin
Copy link
Contributor

@haijian-vaadin haijian-vaadin commented Apr 14, 2020

This change is Reviewable

@haijian-vaadin haijian-vaadin added the hilla Issues related to Hilla label Apr 14, 2020
@@ -153,6 +161,17 @@ public VaadinConnectController(
}
}

private ObjectMapper getDefaultObjectMapper(ApplicationContext context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private ObjectMapper getDefaultObjectMapper(ApplicationContext context) {
private ObjectMapper createVaadinConnectObjectMapper(ApplicationContext context) {
  • I would rename the get to create, because we create a new instance
  • I would rename the method to show that we create the default ObjectMapper for VaadinConnect, not the default ObjectMapper

@@ -153,6 +161,17 @@ public VaadinConnectController(
}
}

private ObjectMapper getDefaultObjectMapper(ApplicationContext context) {
ObjectMapper objectMapper = Jackson2ObjectMapperBuilder.json().build();
Copy link
Contributor

@knoobie knoobie Apr 14, 2020

Choose a reason for hiding this comment

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

Edit: This could be all wrong - let me check it.. just saw that it is a static method call.

Edit2: It is working like this (it inherits in my test spring's config) - this is surprising.. but I it feels wrong. I would still be in favor ofcontext.getBean(Jackson2ObjectMapperBuilder.class) - just in case.

This won't work Because you create a completely new instance again (this time from the Jackson2ObjectMapperBuilder) - so all spring related defaults are gone again.

This can be fixed in two ways:

  • add Jackson2ObjectMapperBuilder to the constructor of the class and in this method as argument
  • context.getBean(Jackson2ObjectMapperBuilder.class) could be used to get the current impl of spring.

See here for more information what is configured: https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jackson/JacksonAutoConfiguration.java#L172

}

@Test
public void should_NotOverrideVisibility_When_JacksonPropertiesProvideVisibility() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking, just my personal opinion: I feel this needs a lot more tests.

ptdatkhtn
ptdatkhtn previously approved these changes Apr 15, 2020
Copy link
Contributor

@ptdatkhtn ptdatkhtn left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 3 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @haijian-vaadin)

Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale (waiting on @knoobie)


flow-server/src/main/java/com/vaadin/flow/server/connect/VaadinConnectController.java, line 164 at r1 (raw file):

Previously, knoobie (Knoobie) wrote…
    private ObjectMapper createVaadinConnectObjectMapper(ApplicationContext context) {
  • I would rename the get to create, because we create a new instance
  • I would rename the method to show that we create the default ObjectMapper for VaadinConnect, not the default ObjectMapper

Done.


flow-server/src/main/java/com/vaadin/flow/server/connect/VaadinConnectController.java, line 165 at r1 (raw file):

Previously, knoobie (Knoobie) wrote…

Edit: This could be all wrong - let me check it.. just saw that it is a static method call.

Edit2: It is working like this (it inherits in my test spring's config) - this is surprising.. but I it feels wrong. I would still be in favor ofcontext.getBean(Jackson2ObjectMapperBuilder.class) - just in case.

This won't work Because you create a completely new instance again (this time from the Jackson2ObjectMapperBuilder) - so all spring related defaults are gone again.

This can be fixed in two ways:

  • add Jackson2ObjectMapperBuilder to the constructor of the class and in this method as argument
  • context.getBean(Jackson2ObjectMapperBuilder.class) could be used to get the current impl of spring.

See here for more information what is configured: https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jackson/JacksonAutoConfiguration.java#L172

hmm, I think I would prefer to static way since it's a static method. Otherwise, I get code warnings.


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerTest.java, line 822 at r1 (raw file):

Previously, knoobie (Knoobie) wrote…

Not blocking, just my personal opinion: I feel this needs a lot more tests.

Done. Now added tests for the reported cases.

@haijian-vaadin
Copy link
Contributor Author

Thank you @knoobie for watching this closely and all your good suggestions. Now I also feel more confident with the tests. Sorry I was a bit busy setting up the tests and didn't reply in time.

Regarding the "/api/get", let's keep it as it is for now, one benefit is that it's used exactly the same as in the reported ticket. Later on, if/when we add more tests, then we can figure a better name.

Copy link
Contributor

@knoobie knoobie left a comment

Choose a reason for hiding this comment

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

LGTM! All good! The end result matters the most!

Reviewed 3 of 4 files at r1, 7 of 7 files at r2.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale

Copy link
Contributor

@knoobie knoobie left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained, and 1 stale

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained, and 1 stale (waiting on @haijian-vaadin)


flow-server/src/test/java/com/vaadin/flow/server/connect/typeconversion/BaseTypeConversionIT.java, line 99 at r2 (raw file):

    }

    protected MockHttpServletResponse callMethod(String methodName) throws Exception {

Why do you need this ?

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained, and 1 stale (waiting on @haijian-vaadin)

Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained, and 2 stale (waiting on @manolo)


flow-server/src/test/java/com/vaadin/flow/server/connect/typeconversion/BaseTypeConversionIT.java, line 99 at r2 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

Why do you need this ?

Good catch, some leftover code that I forgot to remove. Done.

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 6 of 7 files at r2, 1 of 1 files at r3.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained, and 2 stale (waiting on @haijian-vaadin and @manolo)


flow-server/src/test/java/com/vaadin/flow/server/connect/rest/BeanWithPrivateFields.java, line 40 at r3 (raw file):

		this.firstName = firstName;
	}
}

pls format following vaadin conventions. In this case replace tabs with 4 spaces, add a new line at the end of the file


flow-server/src/test/java/com/vaadin/flow/server/connect/rest/EndpointWithRestControllerTest.java, line 118 at r3 (raw file):

        return mockMvcForEndpoint.perform(requestBuilder).andReturn().getResponse().getContentAsString();
    }
}

nit: add new line


flow-server/src/test/java/com/vaadin/flow/server/connect/rest/MyRestController.java, line 32 at r3 (raw file):

	}

}

format file


flow-server/src/test/java/com/vaadin/flow/server/connect/rest/VaadinConnectEndpoints.java, line 39 at r3 (raw file):

            return zonedDateTime;
        }
    

extra spaces

@manolo manolo self-requested a review April 16, 2020 09:50
Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained, and 2 stale (waiting on @haijian-vaadin, @knoobie, and @manolo)


flow-server/src/test/java/com/vaadin/flow/server/connect/rest/MyRestController.java, line 32 at r3 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

format file

Done.


flow-server/src/test/java/com/vaadin/flow/server/connect/rest/VaadinConnectEndpoints.java, line 39 at r3 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

extra spaces

Done.


flow-server/src/test/java/com/vaadin/flow/server/connect/typeconversion/BaseTypeConversionIT.java, line 99 at r2 (raw file):

Previously, haijian-vaadin (Haijian Wang) wrote…

Good catch, some leftover code that I forgot to remove. Done.

Done.

Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained, and 2 stale (waiting on @knoobie and @manolo)


flow-server/src/test/java/com/vaadin/flow/server/connect/rest/BeanWithPrivateFields.java, line 40 at r3 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

pls format following vaadin conventions. In this case replace tabs with 4 spaces, add a new line at the end of the file

Done.


flow-server/src/test/java/com/vaadin/flow/server/connect/rest/EndpointWithRestControllerTest.java, line 118 at r3 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

nit: add new line

Done.

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r4.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained, and 2 stale

@manolo manolo merged commit e9e27c3 into master Apr 16, 2020
@manolo manolo deleted the haijian/fix-object-mapper branch April 16, 2020 12:31
haijian-vaadin added a commit that referenced this pull request Apr 16, 2020
…8051)

* inherit the default behaviour of spring for internal object mapper

* fix test

* add tests

* rest endpoint test refactor

* revert VaadinConnectTypeConversionEndpoints.java

* Remove the IT suffix from the test

* method renaming

* remove leftover code

* code formatting

* code format

* code formatting
haijian-vaadin added a commit that referenced this pull request Apr 16, 2020
…8051)

* inherit the default behaviour of spring for internal object mapper

* fix test

* add tests

* rest endpoint test refactor

* revert VaadinConnectTypeConversionEndpoints.java

* Remove the IT suffix from the test

* method renaming

* remove leftover code

* code formatting

* code format

* code formatting
haijian-vaadin added a commit that referenced this pull request Apr 16, 2020
…8051)

* inherit the default behaviour of spring for internal object mapper

* fix test

* add tests

* rest endpoint test refactor

* revert VaadinConnectTypeConversionEndpoints.java

* Remove the IT suffix from the test

* method renaming

* remove leftover code

* code formatting

* code format

* code formatting
haijian-vaadin added a commit that referenced this pull request Apr 16, 2020
…8051)

* inherit the default behaviour of spring for internal object mapper

* fix test

* add tests

* rest endpoint test refactor

* revert VaadinConnectTypeConversionEndpoints.java

* Remove the IT suffix from the test

* method renaming

* remove leftover code

* code formatting

* code format

* code formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hilla Issues related to Hilla +1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants