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

Introduce overloaded MockPart constructor that accepts the Content-Type #31757

Conversation

HyeongMokJeong
Copy link
Contributor

This PR introduces a constructor to MockPart, enabling the configuration of Content-Type.

When testing controllers that expect a specific format, such as JSON, as @RequestPart, this resolves the issue of encountering a 415 status code problem.

@pivotal-cla
Copy link

@HyeongMokJeong Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@HyeongMokJeong Thank you for signing the Contributor License Agreement!

@HyeongMokJeong HyeongMokJeong reopened this Dec 5, 2023
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 5, 2023
@sbrannen sbrannen added in: test Issues in the test module status: waiting-for-triage An issue we've not yet triaged or decided on in: web Issues in web modules (web, webmvc, webflux, websocket) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 5, 2023
@sbrannen sbrannen changed the title add constructor to MockPart that allows setting the Content-Type Introduce overloaded MockPart constructor that accepts the Content-Type Dec 5, 2023
@sbrannen
Copy link
Member

sbrannen commented Dec 5, 2023

Hi @HyeongMokJeong,

Congratulations on submitting your first PR for the Spring Framework! 👍

Out of curiosity, why do you think the content type warrants a dedicated constructor for MockPart?

Have you considered setting the content type after instantiating the MockPart as follows?

MockPart mockPart = new MockPart("myPart", """
		{"foo": "bar"}
		""".getBytes());

mockPart.getHeaders().setContentType(MediaType.APPLICATION_JSON);

@sbrannen sbrannen added the status: waiting-for-feedback We need additional information before we can continue label Dec 5, 2023
@HyeongMokJeong
Copy link
Contributor Author

HyeongMokJeong commented Dec 5, 2023

Hi @sbrannen

Thank you for revising the title and providing feedback.

I may not have advanced skills, but I think it would be convenient if HttpHeaders could be configured more flexibly.
I understand the benefits of having constrained code, but I'm curious about how it would be in this case(MockPart).
In that context, Can I hear your opinion on initializing HttpHeaders through the constructor?

Like this

    public MockPart(String name, @Nullable String filename, @Nullable byte[] content, HttpHeaders headers) {
        Assert.hasLength(name, "'name' must not be empty");
        this.name = name;
        this.filename = filename;
        this.content = (content != null ? content : new byte[0]);
        this.headers = headers;
    }

@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 Dec 5, 2023
@sbrannen
Copy link
Member

sbrannen commented Dec 5, 2023

In that context, Can I hear your opinion on initializing HttpHeaders through the constructor?

After taking a closer look at jakarta.servlet.http.Part, I think we should in fact introduce an overloaded constructor that allows one to set the content type at the outset.

The rationale is that Part provides first-class support for getting the content type, name, and submitted file name; whereas, MockPart only allows the latter two to be set via a constructor.

So first-class support for the content type is the only attribute missing in terms of symmetry.

As an alternative, we could consider introducing a setContentType(...) method like we have in MockHttpServletRequest, but I think introducing an overloaded constructor is a better option since one always has access to the HttpHeaders via getHeaders() if one wishes to set a specific header after MockPart has been instantiated.

In light of that, let's keep the API minimal and go with your original proposal in this PR.

@sbrannen sbrannen added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Dec 5, 2023
@sbrannen sbrannen self-assigned this Dec 5, 2023
@sbrannen sbrannen added this to the 6.1.2 milestone Dec 5, 2023
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Dec 5, 2023
@sbrannen sbrannen closed this in a596c0e Dec 5, 2023
@sbrannen
Copy link
Member

sbrannen commented Dec 5, 2023

This has been merged into main in a596c0e and revised in db48813.

Thanks

@HyeongMokJeong HyeongMokJeong deleted the add-constructor-to-mock-part branch December 5, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants