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

Optimize the usage of JacksonMongoSessionConverter to prevent duplicate MongoSession Document saves when a custom ObjectMapper is provided. #3185

Open
xiaoquanidea opened this issue Sep 2, 2024 · 4 comments
Assignees
Labels
in: mongo-db status: feedback-provided Feedback has been provided type: enhancement A general enhancement

Comments

@xiaoquanidea
Copy link

While retaining the original instantiation method of JacksonMongoSessionConverter, it should also support the ability to pass in a custom ObjectMapper, with the framework providing basic custom configurations. This approach reduces the cost for users when using a custom ObjectMapper and prevents issues related to document duplication.

While retaining the original constructor, add a new constructor with an additional copyToUse parameter. If copyToUse is set to true, a copy of the ObjectMapper will be created using its copy method, followed by basic configuration. If the user-provided ObjectMapper has already registered some modules, users can adjust the parameters to ensure that module duplication issues are avoided.

  1. If response.write is actively called in loginSuccessHandler, since the native servlet response is wrapped, calling the getWriter method on the wrapped response will return an instance of OnCommittedResponseWrapper.SaveContextPrintWriter. Before this instance calls the flush method, it will invoke the onResponseCommitted method, which will internally perform the first save of the MongoSession.

  2. After the logic in point 1 is completed, the process will continue to the finally block of the SessionRepositoryFilter#doFilterInternal method (where the following code is executed in the finally block: wrappedRequest.commitSession()), and this method will perform the second save of the session.

If the _id property of the Document is the same, the second call to Mongo's save will perform an update operation. When using JdkMongoSessionConverter, there will be no issue of saving the session twice because this converter actively sets the _id. However, in JacksonMongoSessionConverter, if a custom ObjectMapper is provided but the ObjectMapper's PropertyNamingStrategy is not configured and Visibility is not set, it can lead to incorrect data being stored in the database.
企业微信截图_40878484-86d6-4744-b735-ce1eb21fef1e
企业微信截图_75be489a-61ed-4b61-95f6-19cd37000073

image

This might confuse those unfamiliar with the Spring Security framework, as they may wonder why there are two records in Mongo.

@xiaoquanidea xiaoquanidea added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Sep 2, 2024
xiaoquanidea added a commit to xiaoquanidea/spring-session that referenced this issue Sep 2, 2024
…te MongoSession Document saves when a custom ObjectMapper is provided.spring-projects#3185
xiaoquanidea added a commit to xiaoquanidea/spring-session that referenced this issue Sep 3, 2024
…te MongoSession Document saves when a custom ObjectMapper is provided.spring-projects#3185
xiaoquanidea added a commit to xiaoquanidea/spring-session that referenced this issue Sep 3, 2024
…te MongoSession Document saves when a custom ObjectMapper is provided.spring-projects#3185
@xiaoquanidea
Copy link
Author

The MongoSession class in the AbstractMongoSessionConverter abstract method did not have the public modifier, which prevented users from extending and implementing it in their own projects. Therefore, I added the public modifier to the MongoSession class.

@marcusdacoregio marcusdacoregio self-assigned this Sep 4, 2024
@marcusdacoregio marcusdacoregio added in: mongo-db and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 4, 2024
@marcusdacoregio
Copy link
Contributor

Hi @xiaoquanidea, thanks for the report.

However, I don't think it should be addressed like you did in #3187. You can always copy the ObjectMapper before passing it to the converter. In addition to that, it is also not possible to not apply the default ObjectMapper configuration.

I think the best approach would be to make the configureObjectMapper method public static and then users could invoke it before passing the ObjectMapper to the converter. What do you think?

@marcusdacoregio marcusdacoregio added the status: waiting-for-feedback We need additional information before we can continue label Sep 4, 2024
@xiaoquanidea
Copy link
Author

Hi @xiaoquanidea, thanks for the report.

However, I don't think it should be addressed like you did in #3187. You can always copy the ObjectMapper before passing it to the converter. In addition to that, it is also not possible to not apply the default ObjectMapper configuration.

I think the best approach would be to make the configureObjectMapper method public static and then users could invoke it before passing the ObjectMapper to the converter. What do you think?

Your approach sounds better; I'll make some adjustments.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 6, 2024
@xiaoquanidea
Copy link
Author

Hi @xiaoquanidea, thanks for the report.

However, I don't think it should be addressed like you did in #3187. You can always copy the ObjectMapper before passing it to the converter. In addition to that, it is also not possible to not apply the default ObjectMapper configuration.

I think the best approach would be to make the configureObjectMapper method public static and then users could invoke it before passing the ObjectMapper to the converter. What do you think?

Hello, I’ve adjusted the code according to the new approach. Please take a look when you have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: mongo-db status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants