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.

Sign up for GitHub

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

Jump to bottom

Parameterise the XML-RPC parse method with the Media Type #5068

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Parameterise the XML-RPC parse method with the Media Type #5068

merged 1 commit into from
Sep 25, 2023

Conversation

ohumbel
Copy link
Contributor

@ohumbel ohumbel commented Sep 22, 2023 •
edited by adamretter
Loading

Description:

Closes #5067.

Reference:

Since XML resources are always and only stored via the parse() method, we need to pass the Media Type as a parameter to this method.

Type of tests:

A unit test in exist-core/src/test/java/org/exist/xmlrpc: MimeTypeTest

I successfully ran the full maven reactor locally.

@reinhapa reinhapa requested a review from adamretter September 22, 2023 14:57
@reinhapa reinhapa added bug issue confirmed as bug needs 6.x.x backport labels Sep 22, 2023
@reinhapa reinhapa force-pushed the bugfix/xmlprc5067 branch 2 times, most recently from e38288a to 9641ef2 Compare September 23, 2023 06:19
@reinhapa reinhapa requested a review from adamretter September 23, 2023 07:00
@ohumbel
Copy link
Contributor Author

ohumbel commented Sep 23, 2023

@reinhapa Thanks for the feature!

@ohumbel
Copy link
Contributor Author

ohumbel commented Sep 23, 2023

Regarding backport to 6.x.x: I think it is necessary, because:

  1. We initially found and fixed the problem in 6.2.0
  2. The slightly modified MimeTypeTest also fails on 6.x.x with:
    [ERROR] MimeTypeTest.testXMLMimeType:68 expected:<[XML]Resource> but was:<[Binary]Resource>

MimeTypeTest.java.txt

The relevant parts are:
XMLResource resource = (XMLResource)collection.createResource(DOCUMENT_NAME, XMLResource.RESOURCE_TYPE);
and
assertEquals(XMLResource.RESOURCE_TYPE, resource.getResourceType());

@reinhapa
Copy link
Member

@adamretter could you do review my enhancement adding the mime type again?

@reinhapa reinhapa requested a review from dizzzz September 23, 2023 08:59
@ohumbel ohumbel mentioned this pull request Sep 23, 2023
Closed
adamretter
adamretter approved these changes Sep 23, 2023
View reviewed changes
Copy link
Contributor

@adamretter adamretter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@adamretter adamretter changed the title Bugfix for xmlprc issue #5067 Parameterise the XML-RPC parse method with the Media Type Sep 25, 2023
@adamretter
Copy link
Contributor

adamretter commented Sep 25, 2023 •
edited
Loading

@ohumbel I think we are just awaiting a backport to 6.x.x? Are backports to 5.x.x and 4.x.x also needed?

@ohumbel @reinhapa
[feature] Provide mime type on store XMLResource
23c5a86 
The mime type that is available on the remote resource was not provided when
calling the parse() procedure on the backend and has now been added.

When calculating the mime type being stored the following order will be used:

 - mime type as given by the method call
 - mime type caclulated based on the file name extension
 - default mime type 'application/octet-stream' is used

Co-authored-by: Otmar Humbel <[email protected]>
Signed-off-by: Patrick Reinhart <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

92.3% 92.3% Coverage
0.0% 0.0% Duplication

@ohumbel
Copy link
Contributor Author

ohumbel commented Sep 25, 2023 •
edited
Loading

@ohumbel I think we are just awaiting a backport to 6.x.x? Are backports to 5.x.x and 4.x.x also needed?

@adamretter I know for sure that backport to 6.x.x is necessary - I'll try to take care of that as soon as the develop one is merged. For 4.x.x, I know for sure that no backport is necessary.
For 5.x.x I think the backport would be needed, because RpcConnection.java looks almost the same as 6.x.x. But I am currently unable to run mvn test on 5.x.x.

@adamretter
Copy link
Contributor

I'll try to take care of that as soon as the develop one is merged

Actually it's better to provide the backports before we merge please.... Otherwise they tend to get mixed up or forgotten!

@ohumbel ohumbel mentioned this pull request Sep 25, 2023
Merged
@reinhapa reinhapa merged commit 31e79b5 into eXist-db:develop Sep 25, 2023
@ohumbel ohumbel deleted the bugfix/xmlprc5067 branch September 25, 2023 15:42
@ohumbel
Copy link
Contributor Author

ohumbel commented Sep 25, 2023

Actually it's better to provide the backports before we merge please.... Otherwise they tend to get mixed up or forgotten!

Both PR's (#5070 and #5071) are now created.

@adamretter adamretter added bug issue confirmed as bug and removed bug issue confirmed as bug needs 6.x.x backport labels Sep 25, 2023
@adamretter adamretter added this to the eXist-7.0.0 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reinhapa reinhapa reinhapa approved these changes

@dizzzz dizzzz dizzzz approved these changes

@adamretter adamretter adamretter approved these changes

Assignees
No one assigned
Labels
bug issue confirmed as bug
Projects
None yet
Milestone
eXist-7.0.0
Development

Successfully merging this pull request may close these issues.

[BUG] Xmlrpc: XML documents without an .xml extension are stored as binary
4 participants
@ohumbel @reinhapa @adamretter @dizzzz