SOLR-15546: ShortestPathStream to handle ids with colons. #236

cpoerschke · 2021-07-26T18:59:26Z

https://issues.apache.org/jira/browse/SOLR-15546

madrob · 2021-07-28T17:31:42Z

solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java

@@ -433,7 +433,7 @@ public JoinRunner(List<String> nodes) {
      StringBuffer nodeQuery = new StringBuffer();

      for(String node : nodes) {
-        nodeQuery.append(node).append(" ");
+        nodeQuery.append('"').append(node).append('"').append(" ");


What happens if there are quotes in the id?

Good remark

What happens if there are quotes in the id?

Good question! I'd briefly wondered about the backwards compatibility aspects w.r.t. adding surrounding quotes in the code when the caller already added them as a workaround, and then I had lost track of that.

One difference between the code adding surrounding quotes vs. the caller adding them is that the code also adds them for interim nodes. I've amended the test coverage to explore that further.

Also adjusted the code change to not add surrounding quotes if there are some already but obviously quotes could also be in the middle of the id, say Alice ("Ally") or Robert ("Bob") both of which also contain parentheses as it happens, hmm.

This reverts commit a633f75.

madrob · 2021-07-29T17:11:22Z

I didn’t even consider the “quoted” case, I was thinking of text with a quote character in the middle. But I’m glad that we are thinking about that one too!

cpoerschke · 2021-07-29T17:16:23Z

... text with a quote character in the middle. ...

I would speculate that somehow encoding the id might be one approach? The text could contain quotes in the middle or apostrophe or colon or space or other stuff too.

This reverts commit e4df590.

…urrounding quotes back compat)

cpoerschke · 2021-07-30T18:40:29Z

Hmm, have tried but not yet found a way to support quotes in the middle.

And if there is a space in the text then the code adding surrounding quotes changes the results, so with that behaviour change in mind perhaps any fix would be for 9x only and then no need to consider the scenario of the caller having added surrounding quotes since backwards compatibility is not retained anyhow?

Either way, I've amended the test coverage to include space and single and double quotes.

madrob · 2021-08-26T16:58:48Z

perhaps any fix would be for 9x only and then no need to consider the scenario of the caller having added surrounding quotes since backwards compatibility is not retained anyhow

I think we still want to back port a fix for colons at least to 8x? Maybe there are parts that stay 9x only, but I think this is probably something important to have in our release line

cpoerschke · 2021-08-26T17:13:30Z

perhaps any fix would be for 9x only and then no need to consider the scenario of the caller having added surrounding quotes since backwards compatibility is not retained anyhow

I think we still want to back port a fix for colons at least to 8x? Maybe there are parts that stay 9x only, but I think this is probably something important to have in our release line

One way for a backwards compatible or "backwards compatible friendly" change for the 8x release line could be via an optional flag:

shortestPath(
  collection,
  from="[email protected]",
  to="[email protected]",
  ...
)

or

shortestPath(
  collection,
  from="[email protected]",
  to="[email protected]",
  ...
  solr15546fix=true
)

for fixed behaviour with the option of

shortestPath(
  collection,
  from="[email protected]",
  to="[email protected]",
  ...
  solr15546fix=false
)

for backwards compatible behaviour. So solr15546fix as an opt-out, solr15546fix would have to be some other name of course, and depending on name and whether or not it would be in 9.x also it could also be opt-in rather than opt-out though for this type of fix opt-out seems best.

solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java

github-actions · 2024-02-25T00:00:32Z

This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the [email protected] mailing list. Thank you for your contribution!

github-actions · 2024-10-06T00:00:48Z

This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.

epugh · 2024-10-06T11:43:01Z

@cpoerschke if we assume this code would only go on main, does that make it easier to merge this code? Seems like a great fix.

github-actions · 2024-12-09T00:00:40Z

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

epugh · 2024-12-09T12:56:52Z

I'd love to see this get into 10 without the legacyJoin stuff, since that seems hard to explain? Thoughts? If you don't have bandwidth/energy on this one, let me know and I can take it on..

cpoerschke · 2025-01-06T13:14:41Z

I'd love to see this get into 10 without the legacyJoin stuff, since that seems hard to explain? Thoughts? If you don't have bandwidth/energy on this one, let me know and I can take it on..

Sure, please feel free to take this over etc. -- thanks!

epugh · 2025-02-15T21:55:55Z

I've tried to take a stab at this, and honestly I think I made this one worse. I think this is going to be one of those "someday if I need this, or a smarter person comes along with a fix, let's look at it again" issue.. Thoughts on adding a warning to the docs to the effect the id's can't have colons instead?

SOLR-15546: ShortestPathStream to handle ids with colons.

a633f75

madrob reviewed Jul 28, 2021

View reviewed changes

cpoerschke added 3 commits July 29, 2021 17:40

Revert "SOLR-15546: ShortestPathStream to handle ids with colons."

add797e

This reverts commit a633f75.

add tests

a6d00ce

change code

e4df590

cpoerschke added 3 commits July 30, 2021 18:44

Revert "change code"

66460c7

This reverts commit e4df590.

add test coverage (space or quotes in id) and reduce test coverage (s…

d72d82c

…urrounding quotes back compat)

change code (more simply)

f18d80e

madrob mentioned this pull request Aug 4, 2021

SOLR-15576: Allow filtering on ISO-8601 formatted timestamp literals in SQL WHERE clause #247

Merged

7 tasks

cpoerschke added 2 commits August 26, 2021 20:38

add legacyJoin boolean flag

3440e35

Merge remote-tracking branch 'origin/main' into SOLR-15546

4252374

cpoerschke commented Aug 26, 2021

View reviewed changes

solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java Outdated Show resolved Hide resolved

solr/solrj/src/java/org/apache/solr/client/solrj/io/graph/ShortestPathStream.java Outdated Show resolved Hide resolved

Apply suggestions from (self) code review

a058377

github-actions bot added the stale PR not updated in 60 days label Feb 25, 2024

github-actions bot added the closed-stale Closed after being stale for 60 days label Oct 6, 2024

github-actions bot closed this Oct 6, 2024

epugh reopened this Oct 6, 2024

github-actions bot added client:solrj tests and removed closed-stale Closed after being stale for 60 days stale PR not updated in 60 days labels Oct 6, 2024

github-actions bot added the stale PR not updated in 60 days label Dec 9, 2024

github-actions bot removed the stale PR not updated in 60 days label Dec 10, 2024

epugh self-assigned this Jan 6, 2025

Merge branch 'main' into pr/236

06d6bb8

epugh removed their assignment Feb 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-15546: ShortestPathStream to handle ids with colons. #236

SOLR-15546: ShortestPathStream to handle ids with colons. #236

cpoerschke commented Jul 26, 2021

madrob Jul 28, 2021

KennyKnecht Jul 29, 2021

cpoerschke Jul 29, 2021

madrob commented Jul 29, 2021

cpoerschke commented Jul 29, 2021

cpoerschke commented Jul 30, 2021

madrob commented Aug 26, 2021

cpoerschke commented Aug 26, 2021

github-actions bot commented Feb 25, 2024

github-actions bot commented Oct 6, 2024

epugh commented Oct 6, 2024

github-actions bot commented Dec 9, 2024

epugh commented Dec 9, 2024

cpoerschke commented Jan 6, 2025

epugh commented Feb 15, 2025

SOLR-15546: ShortestPathStream to handle ids with colons. #236

Are you sure you want to change the base?

SOLR-15546: ShortestPathStream to handle ids with colons. #236

Conversation

cpoerschke commented Jul 26, 2021

madrob Jul 28, 2021

Choose a reason for hiding this comment

KennyKnecht Jul 29, 2021

Choose a reason for hiding this comment

cpoerschke Jul 29, 2021

Choose a reason for hiding this comment

madrob commented Jul 29, 2021

cpoerschke commented Jul 29, 2021

cpoerschke commented Jul 30, 2021

madrob commented Aug 26, 2021

cpoerschke commented Aug 26, 2021

github-actions bot commented Feb 25, 2024

github-actions bot commented Oct 6, 2024

epugh commented Oct 6, 2024

github-actions bot commented Dec 9, 2024

epugh commented Dec 9, 2024

cpoerschke commented Jan 6, 2025

epugh commented Feb 15, 2025