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

MultiPartConfig in HTTP Vert.x extension is insufficiently documented #30046

Closed
michalvavrik opened this issue Dec 22, 2022 · 9 comments · Fixed by #30214
Closed

MultiPartConfig in HTTP Vert.x extension is insufficiently documented #30046

michalvavrik opened this issue Dec 22, 2022 · 9 comments · Fixed by #30214
Labels
area/rest kind/bug Something isn't working
Milestone

Comments

@michalvavrik
Copy link
Member

michalvavrik commented Dec 22, 2022

Describe the bug

Hello,

I have one opinion (you can ignore that one) and one concern (IMO bug) regarding #29729 documentation.

Opinion: Please document this feature in more detail and ideally provide an example in docs. Documentation does not provide single example on MultiPartConfig and fileContentTypes. I understand you (and me) know what MultiPartConfig#fileContentType should be used for because we read this PR, but users didn't not, they only see this configuration property with text A list of {@code ContentType} to indicate whether a given multipart field should be handled as a file part.
With context of #29729 that sentence is completely clear, but you can only find this in configuration properties list.

Concern This PR added MultiPartConfig to Vert.x HTTP extension. MultiPartConfig only used by RESTEasy Reactive and ignored by every other extension (RESTEasy Classic, Reactive Routes, ...) while that fact is literally nowhere mentioned.
How is it possible to have the configuration in Vert.X HTTP extension and do not mention it only works for RESTEasy Reactive? How should user know that?

Expected behavior

one of:

  • I expect MultiPartConfig#fileContentType configuration property works for RESTEasy Classic, Reactive Routes etc.
  • Document that it only works for RESTEasy Reactive

Actual behavior

  • only works for RESTEasy Reactive
  • docs don't tell me that the property only works for single extension

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 22, 2022

/cc @FroMage(resteasy-reactive), @geoand(resteasy-reactive), @stuartwdouglas(resteasy-reactive)

@michalvavrik
Copy link
Member Author

/cc @pedroigor @gsmet as you were participating on the PR.

@geoand
Copy link
Contributor

geoand commented Dec 23, 2022

You are right, I totally missed this...

We probably do need to mention that it only works for RESTEasy Reactive (currently)

@rsvoboda
Copy link
Member

And the fix for this issue needs to be back-ported to 2.13 branch as #29729 was back-ported to 2.13.6.Final

@pedroigor
Copy link
Contributor

@geoand Do you want me to make the change?

@geoand
Copy link
Contributor

geoand commented Jan 3, 2023

@pedroigor sure, that would be nice

@pedroigor
Copy link
Contributor

pedroigor commented Jan 3, 2023

I'm trying to improve the documentation for the option:

A comma-separated list of {@code ContentType} to indicate whether a given multipart field should be handled as a file part. You can use this setting to force HTTP-based extensions to parse a message part as a file based on its content type.

?This setting only works if you are using Resteasy Reactive?

I'm kinda stuck trying to explain that it only works for RR because the setting (as mentioned by @michalvavrik) is a top-level HTTP setting. Looks like the right thing to do is to deprecate this one and move the option to RR, but I'm not sure if we can fix that now ...

Otherwise, if you think we can just mention RR in the documentation above it works for me too.

@geoand
Copy link
Contributor

geoand commented Jan 4, 2023

but I'm not sure if we can fix that now ...

Yeah, I think it's too late for that.

Otherwise, if you think we can just mention RR in the documentation above it works for me too

I'm fine with this

@pedroigor
Copy link
Contributor

@geoand @michalvavrik Not ideal. Can you please check if it is better [1]?

[1] #30214

@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Jan 9, 2023
@gsmet gsmet modified the milestones: 2.16 - main, 2.13.7.Final Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants