You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/10/25 18:30:55 UTC
[GitHub] [solr] stillalex opened a new pull request, #1127: SOLR-16477 Collection RENAME api creates broken alias
stillalex opened a new pull request, #1127:
URL: https://github.com/apache/solr/pull/1127
https://issues.apache.org/jira/browse/SOLR-16477
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Collection RENAME api creates broken alias, named as source pointing to collection named as target.
# Solution
Revert params to create alias as 'target' pointing to source collection.
# Tests
Unit tests for Aliases class covering the rename.
I had a look at a failing test under CollectionsAPISolrJTest and had to tweak the assertions a bit, would really appreciate some input if the changes make sense or not.
Just to add to this, I'm currently not sure if a client going through SolrJ can issue a rename which has a different path than the api I changed, where the expectation is different, or this test is just emulating the code as it was written and effectively mirroring the bug.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1127:
URL: https://github.com/apache/solr/pull/1127#issuecomment-1292413300
> @stillalex I noticed in `Aliases.java` line 322 - still has `throw new NullPointerException("Alias name cannot be null");`
>
> Is it possible this should be addressed as well? I think based on your new tests it might be possible to add a test to fix this case as well? null for an alias?
I avoided doing too much cleanup/refactoring outside of the reported bug, but I can include this change as well in the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1127:
URL: https://github.com/apache/solr/pull/1127#issuecomment-1292417417
> I avoided doing too much cleanup/refactoring outside of the reported bug, but I can include this change as well in the PR.
Agreed makes sense, but I think in this case if you add a test case for `SolrException e = assertThrows(SolrException.class, () -> aliases.cloneWithRename("alias1", null));` you might trigger that other NPE case? At least from a quick glance that seemed related to the other NPE test case you added.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
stillalex commented on code in PR #1127:
URL: https://github.com/apache/solr/pull/1127#discussion_r1004897653
##########
solr/CHANGES.txt:
##########
@@ -65,6 +65,8 @@ Optimizations
Bug Fixes
---------------------
+* SOLR-16477: Collection RENAME api creates broken alias
Review Comment:
moved it to the bottom
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1127:
URL: https://github.com/apache/solr/pull/1127#issuecomment-1292449259
@risdenk added the NPE change + some more tests. shouldn't be any breaking test, but I am running the complete test suite again locally for validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1127:
URL: https://github.com/apache/solr/pull/1127#discussion_r1004878560
##########
solr/solrj/src/test/org/apache/solr/common/cloud/AliasesTest.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.common.cloud;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.common.SolrException;
+
+/** Unit test for {@link Aliases} */
+public class AliasesTest extends SolrTestCase {
+
+ public void testCloneWithRenameNullBefore() {
+ Aliases aliases = Aliases.EMPTY;
+ try {
+ aliases.cloneWithRename(null, "alias1");
Review Comment:
`assertThrows` would be better for this test?
##########
solr/solrj/src/test/org/apache/solr/common/cloud/AliasesTest.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.common.cloud;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.common.SolrException;
+
+/** Unit test for {@link Aliases} */
+public class AliasesTest extends SolrTestCase {
+
+ public void testCloneWithRenameNullBefore() {
+ Aliases aliases = Aliases.EMPTY;
+ try {
+ aliases.cloneWithRename(null, "alias1");
+ fail("null value should not be allowed");
+ } catch (SolrException e) {
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ }
+ }
+
+ public void testCloneWithRenameEmptyBefore() {
+ Aliases aliases = Aliases.EMPTY;
+ try {
+ aliases.cloneWithRename("", "alias1");
+ fail("null value should not be allowed");
+ } catch (SolrException e) {
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ }
+ }
+
+ public void testCloneWithRenameEmptyAfter() {
+ Aliases aliases = Aliases.EMPTY;
+ try {
+ aliases.cloneWithRename("alias0", "");
+ fail("null value should not be allowed");
+ } catch (SolrException e) {
Review Comment:
`assertThrows` would be better for this test?
##########
solr/CHANGES.txt:
##########
@@ -65,6 +65,8 @@ Optimizations
Bug Fixes
---------------------
+* SOLR-16477: Collection RENAME api creates broken alias
Review Comment:
nit: usually this goes at the bottom of the list.
##########
solr/solrj/src/test/org/apache/solr/common/cloud/AliasesTest.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.common.cloud;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.solr.SolrTestCase;
+import org.apache.solr.common.SolrException;
+
+/** Unit test for {@link Aliases} */
+public class AliasesTest extends SolrTestCase {
+
+ public void testCloneWithRenameNullBefore() {
+ Aliases aliases = Aliases.EMPTY;
+ try {
+ aliases.cloneWithRename(null, "alias1");
+ fail("null value should not be allowed");
+ } catch (SolrException e) {
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+ }
+ }
+
+ public void testCloneWithRenameEmptyBefore() {
+ Aliases aliases = Aliases.EMPTY;
+ try {
+ aliases.cloneWithRename("", "alias1");
+ fail("null value should not be allowed");
+ } catch (SolrException e) {
Review Comment:
`assertThrows` would be better for this test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1127:
URL: https://github.com/apache/solr/pull/1127#issuecomment-1291053356
thanks for taking a look @risdenk! updated PR based on your observations so far.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1127:
URL: https://github.com/apache/solr/pull/1127#issuecomment-1292340567
thank you @gerlowskija for venturing a guess :) even though benign, having this dead code around adds a bit of cognitive complexity when trying to understand what it actually does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1127:
URL: https://github.com/apache/solr/pull/1127#issuecomment-1292362469
@stillalex maybe I'm old school, but I think the method looks better written without streams:
```
public static Map<String, List<String>> convertMapOfCommaDelimitedToMapOfList(
Map<String, String> collectionAliasMap) {
Map<String, List<String>> map = new LinkedHashMap<>();
for (Map.Entry<String, String> entry : collectionAliasMap.entrySet()) {
map.put(entry.getKey(), splitCollections(entry.getValue()));
}
return map;
}
```
assuming I converted that back correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on PR #1127:
URL: https://github.com/apache/solr/pull/1127#issuecomment-1291967379
> if the starting point for the stream is a Map<>, the 'merge keys' part where it checks for collisions(a, b) -> IllegalStateException would never be possible
In this case, I think that's true - we're not really "mapping" the keys in the sense this method allows, we're just taking them unmodified from an existing Map. I can imagine cases where the merge/collection function would be necessary, but it looks redundant here to me 🤷
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on pull request #1127: SOLR-16477 Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
stillalex commented on PR #1127:
URL: https://github.com/apache/solr/pull/1127#issuecomment-1291056422
There is some code in the Aliases class I don't get:
```
public static Map<String, List<String>> convertMapOfCommaDelimitedToMapOfList(
Map<String, String> collectionAliasMap) {
return collectionAliasMap.entrySet().stream()
.collect(
Collectors.toMap(
Map.Entry::getKey,
e -> splitCollections(e.getValue()),
(a, b) -> {
throw new IllegalStateException(
String.format(Locale.ROOT, "Duplicate key %s", b));
},
LinkedHashMap::new));
}
```
if the starting point for the stream is a `Map<>`, the 'merge keys' part where it checks for collisions`(a, b) -> IllegalStateException` would never be possible, as the keys are already guaranteed to be unique by the map contract. am I reading this wrong?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on pull request #1127: SOLR-16477: Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
risdenk commented on PR #1127:
URL: https://github.com/apache/solr/pull/1127#issuecomment-1292663214
Thanks @stillalex!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk merged pull request #1127: SOLR-16477: Collection RENAME api creates broken alias
Posted by GitBox <gi...@apache.org>.
risdenk merged PR #1127:
URL: https://github.com/apache/solr/pull/1127
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org