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

fix(airbyte-api): regexp for JobsController orderBy #300

Closed

Conversation

justenwalker
Copy link
Contributor

@justenwalker justenwalker commented Jan 20, 2024

What

The regexp for the orderBy parameter is incorrect since it is using a bare pipe character which implies an alternate expression. However, what is expected is matching a literal pipe '|' character.

Original Regexp

([a-zA-Z0-9]+)|(ASC|DESC)

The expectation is that this will match createdAt|ASC and create the following matching groups:

  1. createdAt
  2. ASC

However, what this regexp actually means is:

original-regexp

# ---- left-hand-side regexp ---
( # start capture group 1
    [a-zA-Z0-9]+ # match alphanumeric string of one or more characters
)
# ---- left-hand-side regexp ---
| # match either left-hand-side OR right-hand-side regexp
# ---- right-hand-side regexp ---
( # start capture group 2
    ASC|DESC # match the literal string `ASC` OR `DESC`
)
# ---- right-hand-side regexp ---

So it will instead produce only:

  1. createdAt
  2. null

Also note: the left-hand-side regexp will match anything the right-hand-side regexp could match, so that branch will never be taken. So the original regexp is functionally equivalent to ([a-zA-Z0-9]+)

How

This change adds a \ escape before the | so that the regexp matches the literal character |.

Fixed Regexp

([a-zA-Z0-9]+)\|(ASC|DESC)

fixed-regexp

This will correctly match the string createdAt|ASC as:

  1. createdAt
  2. ASC

Recommended reading order

  1. airbyte-api-server/src/main/kotlin/io/airbyte/api/server/controllers/JobsController.kt

Can this PR be safely reverted / rolled back?

If you know that your PR is backwards-compatible and can be simply reverted or rolled back, check the YES box.

Otherwise if your PR has a breaking change, like a database migration for example, check the NO box.

If unsure, leave it blank.

  • YES 💚

🚨 User Impact 🚨

No

The regexp for the orderBy parameter is incorrect since it is using a bare pipe character which
implies an alternate expression. However, what is expected is matching a literal pipe '|' character.

This change adds a '\' escape before the '|' so that the regexp matches the literal character.
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@marcosmarxm
Copy link
Member

Thanks @justenwalker requested to the platform team take a look in your pull request.

Copy link
Contributor

@JonsSpaghetti JonsSpaghetti left a comment

Choose a reason for hiding this comment

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

Good catch, thank you!

@sneakersgames
Copy link

@marcosmarxm @JonsSpaghetti When is this merge going on the main branch? We need this fix kinda urgently. 😄 Thanks!

Copy link
Contributor

Your branch is not currently up-to-date with main. Please update your branch before attempting to snapshot your PR.

@CLAassistant
Copy link

CLAassistant commented Feb 20, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ justenwalker
❌ JonsSpaghetti
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
2.0% Duplication on New Code

See analysis details on SonarCloud

@sneakersgames
Copy link

It's stuck?

@JonsSpaghetti
Copy link
Contributor

@sneakersgames Sorry for the delay on this, following up on the process for this it looks like I need to pull it into our internal repo and merge it there to make the Ci runners happy. Will do that early next week.

Copy link
Contributor

github-actions bot commented Mar 2, 2024

Your branch is not currently up-to-date with main. Please update your branch before attempting to snapshot your PR.

@sneakersgames
Copy link

@JonsSpaghetti Any news on the merge?

Copy link
Contributor

Your branch is not currently up-to-date with main. Please update your branch before attempting to snapshot your PR.

@JonsSpaghetti
Copy link
Contributor

@sneakersgames this has been completed and should be present in the latest version

Copy link
Contributor

Your branch is not currently up-to-date with main. Please update your branch before attempting to snapshot your PR.

@justenwalker
Copy link
Contributor Author

Closing as fixed via eb2504a

@justenwalker justenwalker deleted the jwalker/fix-job-list branch March 20, 2024 15:44
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.

5 participants