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

Add header-less methods to MessageBodyReader and ChunkedMessageBodyReader #11374

Closed

Conversation

glorrian
Copy link
Contributor

I think the interfaces we have now are too demanding. Most of the implemented methods of these interfaces don't actually use the headers param, so I think we should provide new interfaces without the headers param.

We also added MessageBodyReader as a replacement forMediaTypeCodec, but we will not be able to replace MediaTypeCodec interface such as <T> T decode(Argument<T> type, InputStream inputStream) method because in many cases where these interface are used there are no headers instance and we also cannot pass it as null reference because headers param is marked with an annotation by @NonNull

@glorrian
Copy link
Contributor Author

@graemerocher @yawkat @dstepanov @sdelamo i'll glad to receive your reviews

@glorrian
Copy link
Contributor Author

and also i don't know why the build is failed. its smth with checkstyle but i checked it all on my local and I dug logs and it all has no errors from files that I changed. all tests passed so I don't have a single clue what's wrong....

@yawkat
Copy link
Member

yawkat commented Nov 25, 2024

What is your use case?

@glorrian
Copy link
Contributor Author

@yawkat The usecase is that I decided to see how I could replace deprecated MediaTypeCodec with a new MessageBodyReader class in the internal code and found out that bodyreader cannot be used in many cases because it requires ‘@nonnull headers’ which typecodec did not require before. So if we want to get rid of typecodec and use bodyreader, we logically need a new method.

@yawkat
Copy link
Member

yawkat commented Nov 25, 2024

What are you using MediaTypeCodec for?

@glorrian
Copy link
Contributor Author

What are you using MediaTypeCodec for?

You can look at jackson impl of the messageBodyReader and its not using headers param at all for example. I just mean that codec interface was like decode(type, bytes) and if we want replace it with messagebodyreader we need same interface like read(type, @nullable mediatype, bytes)

@glorrian
Copy link
Contributor Author

POC of this pr it is a fact that we will NOT be able to replace deprecated mediatypedecoder until we provide an interface that does not require the use of headers, simply because in most usecases headers do NOT exist when we use mediatypecodec.

@yawkat
Copy link
Member

yawkat commented Nov 25, 2024

I understand that MessageBodyReader is not a perfect substitute for MediaTypeCodec. MessageBodyReader was designed specifically for HTTP messages. There are in-framework use cases where this approach does not work (multipart, specifically), and I am considering additional APIs for those cases. For that I want to understand what you are using it for.

@glorrian
Copy link
Contributor Author

I got you. But in the code MediaTypeCodec is marked as deprecated for removal, so we need the replacement to it. If messagebodyreader is not fits to this usecase maybe we need especially new interface?

@dstepanov
Copy link
Contributor

You can use new SimpleHttpHeaders if you don't have headers

@dstepanov
Copy link
Contributor

You can create your own interface that adds a default method with new SimpleHttpHeaders and introduces a method without headers

@glorrian
Copy link
Contributor Author

@dstepanov can i create a new pr with a new interface without headers?

@dstepanov
Copy link
Contributor

I think you can have it as an internal one for your project

@glorrian glorrian closed this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants