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

Support for the "type" parameter for Atom Feed/Entry message conversion [SPR-17040] #21578

Open
spring-issuemaster opened this issue Jul 14, 2018 · 5 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Jul 14, 2018

Mark Hobson opened SPR-17040 and commented

AtomFeedHttpMessageConverter and RssChannelHttpMessageConverter currently discard any media type parameters that are supplied to write(). They are overwritten when adding the charset parameter.


Affects: 4.3.18, 5.0.7

Issue Links:

  • #21670 Use parameters declared in consumes or produces condition to narrow the request mapping

Referenced from: pull request #1885

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 8, 2018

Rossen Stoyanchev commented

Original comment and discussion under #15531.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 8, 2018

Rossen Stoyanchev commented

Mark Hobson, I don't quite see what the proposed change accomplishes.

In light of your comment under #15531 for "type=feed" vs "type=entry", this does not change what gets rendered. If a client requests "application/atom+xml;type=entry", before this change, the response would be "application/atom+xml" with a <feed> in the body. This of course is not what the client asked for, but at least the response content type and the body of the response are consistent with each other, and AtomFeedHttpMessageConverter only supports rendering a Feed in any case.

We could go a step further and prevent the mapping in the first place, see my comment under #21670 to use parameters declared in the produces condition to narrow the request mapping. But can you explain how do you benefit from the change proposed in the attached PR?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 8, 2018

Mark Hobson commented

Apologies for the confusion Rossen Stoyanchev. This PR is related to #15531 but doesn't purport to fix that - I'll comment there regarding content negotiation.

The problem that this PR addresses is this: when Spring's AtomFeedHttpMessageConverter is subclassed to have supportedMediaTypes containing a fully-qualified Atom media type of "application/atom+xml;type=feed" then "type=feed" is written to the response, rather than getting inadvertently removed when appending the charset.

This becomes relevant when an application also has an Atom entry message converter so that they can both write their correct Content-Type "type" parameter.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 16, 2018

Rossen Stoyanchev commented

Okay, I see. In that case if the PR is applied, and with all else being the same (i.e. without your other customization to support type=entry), the AtomFeedHttpMessageConverter will happily render a Feed and set the content type to "application/atom+xml;type=entry".

First, we will try and address #21670 for 5.2 which should make this possible:

@GetMapping(path = "/foo", produces = "application/atom+xml;type=feed")
public Feed getFooFeed() {
    // ...
}

@GetMapping(path = "/foo", produces = "application/atom+xml;type=entry")
public Entry getFooEntry() {
    // ...
}

The above should help narrow the mappings, also taking the declared type parameter in mind. Beyond that it would make sense to add explicit support for the "type" parameter in any Atom-specific sub-classes of AbstractWireFeedHttpMessageConverter (we could add one for Entry too) rather than just copying all parameters. After all it is a parameter specified in the RFC so it makes sense to support it explicitly. For example canRead and canWrite should take into account whether the input MediaType has the type parameter. Also when preparing the MediaType for the response, the base class should probably consult sub-classes if any parameters should be added.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 23, 2018

Mark Hobson commented

That sounds like a good approach, thanks. I'll comment on #21670 with a link to example code for my use-cases.

Shall I raise another issue for adding an Atom feed entry message converter to Spring too? An initial implementation will be in my example code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.