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

[Refactor] MediaTypeParser to MediaTypeParserRegistry #8636

Merged
merged 3 commits into from
Jul 24, 2023

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Jul 11, 2023

Refactors the static MediaTypeParser utility to a dynamic registry to support registration of new MediaType definitions. This is a next step to decoupling the x-content library from core classes in the server module to support modularity and cloud or serverless implementations.

relates #5910
relates #8110

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize nknize force-pushed the mediaTypeParserRegistry branch from 614b0ca to 2b50552 Compare July 11, 2023 20:09
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.remotestore.multipart.RemoteStoreMultipartIT.testStaleCommitDeletionWithInvokeFlush
      1 org.opensearch.indices.replication.SegmentReplicationTargetServiceTests.testShardAlreadyReplicating

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #8636 (c7eacc4) into main (3ee4292) will decrease coverage by 0.18%.
The diff coverage is 69.79%.

@@             Coverage Diff              @@
##               main    #8636      +/-   ##
============================================
- Coverage     71.14%   70.96%   -0.18%     
+ Complexity    57214    57172      -42     
============================================
  Files          4757     4757              
  Lines        269804   269823      +19     
  Branches      39454    39474      +20     
============================================
- Hits         191940   191471     -469     
- Misses        61716    62266     +550     
+ Partials      16148    16086      -62     
Files Changed Coverage Δ
...ch/plugin/noop/action/bulk/RestNoopBulkAction.java 0.00% <0.00%> (ø)
...org/opensearch/action/bulk/BulkRequestBuilder.java 12.50% <0.00%> (ø)
...ensearch/action/bulk/TransportShardBulkAction.java 74.49% <0.00%> (-1.16%) ⬇️
...pensearch/action/update/TransportUpdateAction.java 4.57% <0.00%> (ø)
...ava/org/opensearch/action/update/UpdateHelper.java 73.68% <ø> (ø)
...org/opensearch/common/xcontent/XContentHelper.java 60.09% <ø> (+0.96%) ⬆️
...rch/extensions/rest/RestSendToExtensionAction.java 42.50% <0.00%> (ø)
...earch/index/reindex/ClientScrollableHitSource.java 83.60% <ø> (ø)
...ensearch/index/termvectors/TermVectorsService.java 17.22% <0.00%> (ø)
...st/action/admin/indices/RestCreateIndexAction.java 60.86% <0.00%> (ø)
... and 55 more

... and 431 files with indirect coverage changes

@nknize nknize force-pushed the mediaTypeParserRegistry branch from 2b50552 to 3d8724b Compare July 12, 2023 15:28
nknize added 2 commits July 24, 2023 14:54
Signed-off-by: Nicholas Walter Knize <[email protected]>
Signed-off-by: Nicholas Walter Knize <[email protected]>
@nknize nknize force-pushed the mediaTypeParserRegistry branch from 0516680 to c7eacc4 Compare July 24, 2023 19:54
@nknize nknize requested a review from sohami as a code owner July 24, 2023 19:54
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@monusingh-1
Copy link

Will this be backported to 2.x ?

@Yury-Fridlyand
Copy link

When CreateIndexRequest::mapping is called with non initiated MediaTypeParserRegistry, an NPE is thrown instead of IAE.

public CreateIndexRequest mapping(Map<String, ?> source) {
try {
XContentBuilder builder = XContentFactory.contentBuilder(MediaTypeParserRegistry.getDefaultMediaType());

public static XContentBuilder contentBuilder(MediaType type) throws IOException {
if (type instanceof XContentType) {
return contentBuilder((XContentType) (type));
}
throw new IllegalArgumentException("Content type [" + type.getClass().getName() + "] not supported");

type.getClass() crashes on line 136 above ^, because type is null.

@vmmusings
Copy link
Member

When CreateIndexRequest::mapping is called with non initiated MediaTypeParserRegistry, an NPE is thrown instead of IAE.

public CreateIndexRequest mapping(Map<String, ?> source) {
try {
XContentBuilder builder = XContentFactory.contentBuilder(MediaTypeParserRegistry.getDefaultMediaType());

public static XContentBuilder contentBuilder(MediaType type) throws IOException {
if (type instanceof XContentType) {
return contentBuilder((XContentType) (type));
}
throw new IllegalArgumentException("Content type [" + type.getClass().getName() + "] not supported");

type.getClass() crashes on line 136 above ^, because type is null.

@nknize Whats the reason for removing default media type and introducing a new static function to set it up?

@nknize
Copy link
Collaborator Author

nknize commented Jul 26, 2023

@nknize Whats the reason for removing default media type and introducing a new static function to set it up?

Defaults are still there. The registry needs to be initialized.

The static Singleton was refactored so the core is not tightly coupled to the XContentType enum. This way downstream plugins can dynamically register a custom media type.

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-8636-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3f8e0cd3de219dee302788a3616ec33835055e5f
# Push it to GitHub
git push --set-upstream origin backport/backport-8636-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-8636-to-2.x.

baba-devv pushed a commit to baba-devv/OpenSearch that referenced this pull request Jul 29, 2023
…ject#8636)

Refactors the static MediaTypeParser utility to a dynamic registry to
support registration of new MediaType definitions. This is a next step
to decoupling the x-content library from core classes in the server
module to support modularity and cloud or serverless implementations.

Signed-off-by: Nicholas Walter Knize <[email protected]>
nknize added a commit to nknize/OpenSearch that referenced this pull request Aug 1, 2023
…ject#8636)

Refactors the static MediaTypeParser utility to a dynamic registry to
support registration of new MediaType definitions. This is a next step
to decoupling the x-content library from core classes in the server
module to support modularity and cloud or serverless implementations.

Signed-off-by: Nicholas Walter Knize <[email protected]>
(cherry picked from commit 3f8e0cd)
nknize added a commit that referenced this pull request Aug 1, 2023
Refactors the static MediaTypeParser utility to a dynamic registry to
support registration of new MediaType definitions. This is a next step
to decoupling the x-content library from core classes in the server
module to support modularity and cloud or serverless implementations.

Signed-off-by: Nicholas Walter Knize <[email protected]>
(cherry picked from commit 3f8e0cd)
kaushalmahi12 pushed a commit to kaushalmahi12/OpenSearch that referenced this pull request Sep 12, 2023
…ject#8636)

Refactors the static MediaTypeParser utility to a dynamic registry to
support registration of new MediaType definitions. This is a next step
to decoupling the x-content library from core classes in the server
module to support modularity and cloud or serverless implementations.

Signed-off-by: Nicholas Walter Knize <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
brusic pushed a commit to brusic/OpenSearch that referenced this pull request Sep 25, 2023
…ject#8636)

Refactors the static MediaTypeParser utility to a dynamic registry to
support registration of new MediaType definitions. This is a next step
to decoupling the x-content library from core classes in the server
module to support modularity and cloud or serverless implementations.

Signed-off-by: Nicholas Walter Knize <[email protected]>
Signed-off-by: Ivan Brusic <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…ject#8636)

Refactors the static MediaTypeParser utility to a dynamic registry to
support registration of new MediaType definitions. This is a next step
to decoupling the x-content library from core classes in the server
module to support modularity and cloud or serverless implementations.

Signed-off-by: Nicholas Walter Knize <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch enhancement Enhancement or improvement to existing feature or request v2.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants