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

Fixes #24996: UpdateHttpDatasetTest datasource test get blocked forever #724

Conversation

fanf
Copy link
Member

@fanf fanf commented Jun 12, 2024

https://issues.rudder.io/issues/24996

So, there was two problems:

  • 1/ try in attempt leads to stalling tests

We had that:

        IOResult.attemptZIO(try {
          json.read[JSONAware](path).succeed
        } catch {
          case _: ClassCastException =>
            try { ...

And in case of exception, it was not doing what was expected but failing with the exception. It might be due to a change in the way ZIO.attempt works (since we switched to 2.x around here), but how the hell didn't we caught it before ? Or is it a more recent change in 2.X branch ? (but then, the last upgrade is in 08/2023 on 8.0 branch).

  • 2/ Second problem: we had a .toString in the fallback case, so all values, even when they were map (ie json objects), were changed into string, so quote espacted, then parsed back AS STRING. Which obviously does not work.

And that leads to two questions:

  • if at some point, JSONAware stopped to be able to cast LinkedArrayList to JSONObject, when did it happened (since we don't seems to have change json path version in all 8.0 live)
  • if it never worked, then how the .toString was added ? It seems to have always been there - since 2019 at least.

So that's more question than solution. IT REALLY STRANGE and it will be good to find the root change that lead to that.

@fanf fanf requested a review from clarktsiory June 12, 2024 15:04
Copy link
Contributor

@clarktsiory clarktsiory left a comment

Choose a reason for hiding this comment

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

LGTM, and I agree that this is really weird... At least with this cleaner way, it works !

@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit d2448d8 into Normation:branches/rudder/8.0 Jun 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants