Skip to content

feat(collection): added alias management - #159

Open
y-luis-rojo wants to merge 8 commits into
apache:mainfrom
y-luis-rojo:feature/alias-management
Open

feat(collection): added alias management#159
y-luis-rojo wants to merge 8 commits into
apache:mainfrom
y-luis-rojo:feature/alias-management

Conversation

@y-luis-rojo

Copy link
Copy Markdown

feat(collection): add alias management tools (list, create, delete)

@adityamparikh adityamparikh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @y-luis-rojo for the contribution. Seems like a useful capability to add.
Added inline comments as well as overall comments to address:

  1. Looks like the happy path in unit tests need actual assertions.
  2. Please add an AliasServiceIntegrationTest that uses Testcontainers to test adding, updating, removing aliases on a new or existing collection.
  3. Please update the docs with this new tool.

Comment thread src/main/java/org/apache/solr/mcp/server/collection/AliasService.java Outdated
Comment thread src/main/java/org/apache/solr/mcp/server/collection/AliasService.java Outdated
create-alias destructive and idempotent.
Added tests.
Updated documentation.
@y-luis-rojo

Copy link
Copy Markdown
Author

Thanks @y-luis-rojo for the contribution. Seems like a useful capability to add. Added inline comments as well as overall comments to address:

  1. Looks like the happy path in unit tests need actual assertions.
  2. Please add an AliasServiceIntegrationTest that uses Testcontainers to test adding, updating, removing aliases on a new or existing collection.
  3. Please update the docs with this new tool.

Thanks for the comments. I think all are now addressed.

@adityamparikh

Copy link
Copy Markdown
Contributor

An end to end integration tests AliasServiceIntegrationTest which uses testcontainers still needs to be added.

@y-luis-rojo

Copy link
Copy Markdown
Author

An end to end integration tests AliasServiceIntegrationTest which uses testcontainers still needs to be added.

Sorry, missed to add it. Done.

Comment thread src/main/java/org/apache/solr/mcp/server/collection/AliasService.java Outdated
Comment thread src/main/java/org/apache/solr/mcp/server/collection/AliasService.java Outdated
Comment thread src/main/java/org/apache/solr/mcp/server/collection/AliasResult.java Outdated
- Derive success from response.getStatus() instead of hardcoding true
- Add @jsonformat to AliasResult timestamp for ISO 8601 consistency
- Use separate TEST_COLLECTION_2 in integration test update scenario
@y-luis-rojo
y-luis-rojo force-pushed the feature/alias-management branch from 4068819 to 72df1c5 Compare July 10, 2026 12:56
@adityamparikh

Copy link
Copy Markdown
Contributor

@epugh Looks good to me. Can you please merge?

@epugh

epugh commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

@epugh Looks good to me. Can you please merge?

thanks for the review! Still on vacation, but hoping to get some coding time this week.

@epugh

epugh commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

@y-luis-rojo looks like some lint checks failed...

adityamparikh and others added 2 commits July 16, 2026 22:31
The Build job runs './gradlew classes testClasses spotlessCheck' as a
fail-fast gate, and spotlessJavaCheck rejected the new alias files for
indentation (spaces where the project's format expects tabs). Because
Unit Tests and Integration Tests are gated behind Build, this single
formatting miss blocked the entire pipeline.

No functional change - output of './gradlew spotlessApply'.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: adityamparikh <aditya.m.parikh@gmail.com>
createAlias_multipleAliasesCanExist (@order(5)) asserted that TEST_ALIAS
still pointed at TEST_COLLECTION, but createAlias_updatesExistingAlias
(@order(4)) had already repointed it to TEST_COLLECTION_2 - and asserts
that it did. The expectation therefore contradicted the preceding test
and failed deterministically:

    AliasServiceIntegrationTest > createAlias_multipleAliasesCanExist()
    FAILED java.lang.AssertionError at AliasServiceIntegrationTest.java:119

AliasService itself is correct; only the test expectation was stale.

Rather than just correcting the expected value, this makes the test
assert solely on aliases it creates itself, so it no longer depends on
what an earlier test left TEST_ALIAS pointing at. TEST_ALIAS_2 is still
left in place for the deletion tests that follow; TEST_ALIAS_3 is this
test's own fixture and is cleaned up in the test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: adityamparikh <aditya.m.parikh@gmail.com>
fix(alias): resolve CI failures on apache#159 (spotless + stale test expectation)
@adityamparikh

Copy link
Copy Markdown
Contributor

I approved. @epugh Could we merge?

@epugh epugh self-assigned this Jul 23, 2026
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