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

SDAAP-101: Fix issue with find pattern matching replace #4756

Open
wants to merge 8 commits into
base: hotfix/2.5.3
Choose a base branch
from

Conversation

rbi-aap
Copy link

@rbi-aap rbi-aap commented Feb 17, 2025

  • Resolved problem where find pattern incorrectly matched the replace
  • Improved regex handling to ensure correct replacements
  • Added test cases to verify expected behavior

- Resolved problem where find pattern incorrectly matched the replace
- Improved regex handling to ensure correct replacements
- Added test cases to verify expected behavior
@rbi-aap
Copy link
Author

rbi-aap commented Feb 17, 2025

Hi Andrew and Team, please review, thanks

@MarkLark86 MarkLark86 added this to the 2.5.4 milestone Feb 17, 2025
@MarkLark86
Copy link
Contributor

@tomaskikutis Could you please review this PR, and see if it is also needed in v2.8, 2.9 & develop branches please.
Cheers

@tomaskikutis
Copy link
Member

@rbi-aap if the browser was freezing it was probably due to infinite loop. Why was it causing an infinite loop?

Is your code only handling the infinite loop issue? If not, what other issues is it handling?

I see after your changes, 2 different functions exist - one to replace at index and another to replace all. Could you stick to a single implementation in a similar fashion how it was previously? e.g. when multiple replacements were needed, the function doing a single replacement would be called multiple times.

@rbi-aap
Copy link
Author

rbi-aap commented Feb 18, 2025

@tomaskikutis,
Thanks for the review. I refactored the replacement logic to improve stability, prevent unintended behavior, and ensure correct text updates. Unit tests were added to validate edge cases. Appreciate your time.

Copy link
Member

@tomaskikutis tomaskikutis left a comment

Choose a reason for hiding this comment

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

Could you answer to each of my 3 questions separatelly?

@rbi-aap
Copy link
Author

rbi-aap commented Feb 18, 2025

@tomaskikutis ,Thanks for the review. The infinite loop was caused by unintended offset shifts leading to continuous execution and in turn, possible issues like callback hell, which have been fixed for stability. The update also prevents duplicate replacements and ensures correct text updates. The functions were split to avoid instability, as a single implementation didn’t handle multiple replacements reliably. thanks

- Resolved merge conflicts between HEAD and commit 5950201
- Merged duplicate test cases while keeping the most relevant details
- Added additional test cases for:
  - HIGHLIGHTS_REPLACE_ALL edge cases
  - Handling special characters in search/replace
- Improved assertions to ensure coverage for different scenarios
@rbi-aap
Copy link
Author

rbi-aap commented Feb 19, 2025

Hi Team, can you review more test case for QA with its function, thanks

@tomaskikutis
Copy link
Member

tomaskikutis commented Feb 19, 2025

Can you provide a sample input text, pattern and replacement text which was causing the infinite loop?

The update also prevents duplicate replacements and ensures correct text updates.

Can you specify samples where it was causing duplicate replacements or incorrect updates?


Please rework the tests so they are simpler

  • limit sample text to a single line.
  • Clean up test names so it is clear which use case is being tested. For example - this it not clear at all - "HIGHLIGHTS_FIND_REPLACE_SINGLE_AND_REPLACE_ALL"

Replace all $ with $AUD ,but no change with $AUD - this one is asserting incorrect behavior. It should replace $AUD too with the pattern that you specified.

'I have $100 in my wallet. The total cost is $50, but sometimes it is $AUD60'.replace(/\$/g, '$AUD')
// > 'I have $AUD100 in my wallet. The total cost is $AUD50, but sometimes it is $AUDAUD60'

- Structured tests into Single, All, and Combined sections
- Improved readability with Arrange-Act-Asse pattern and descriptive names
@rbi-aap
Copy link
Author

rbi-aap commented Feb 21, 2025

Hi @tomaskikutis,

Thanks for your review!

  • Infinite loop sample: "$100" with pattern "$" and replacement "$AUD" looped endlessly.
  • Update resolves duplicates and ensures correct replacements.
  • Fixed duplicate sample: "$AUD60" no longer becomes "$AUDAUD60" with "$" → "$AUD".
  • Tests are simpler, using AAA pattern—see new commit.
  • Sample text now single-line, e.g., "Total: $100, cost: $50".
  • Test names improved, e.g., "CombinedReplacementScenarios".
  • Added QA test for edge cases.

Really appreciate your support!

@tomaskikutis
Copy link
Member

I've looked into the issue and indeed there is a bug with an infinite loop. The cause was that the function was performing replacement on text that was already replaced.

For example if text was air and we wanted to replace "air" with "airplane", it would do it correctly the first iteration, but then would search for "air" in "airplane" and would replace to "airplaneplane", then to "airplaneplaneplane" - and would never stop.

I've fixed the infinite loop in #4758 by adjusting code so it does not attempt to replace what was already replaced once.

Besides that - I don't see any other issues.

Your comment about duplicates is describing correct behavior. I've added a test that given text "dev dev dev" and trying to replace "dev" with "developers" it should output "developers developers developers". It's not a bug - that's how "replace all" is supposed to work.

Regarding currency conversion where you want to change "$" to "$AUD" - but keep existing mentions of "$AUD" intact - that can't be done with "find and replace" feature without regex support. I don't suggest to expose regex support directly to end-users either - that'd be an overkill for superdesk users.

The best solution I think would be to have pre-made custom replacement strategies. It could either be integrated into "find and replace" widget or you could use macros. There is one already for USD to CAD conversion.

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.

4 participants