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

Loose BigQuery GCP project ID regex restrictions #32178

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

kberezin-nshl
Copy link
Contributor

This addresses #32168, please see rationale there

Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@kberezin-nshl
Copy link
Contributor Author

assign set of reviewers

@kberezin-nshl
Copy link
Contributor Author

There is probably a test flake?

Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damondouglas for label java.
R: @chamikaramj for label io.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@Abacn
Copy link
Contributor

Abacn commented Aug 16, 2024

I agree this is a reasonable change. Checked that PROJECT_ID_REGEXP is only used for extracting projectid/dataset/table from a table reference or table urn, it is not used in validation purpose.

For this reason please preserve the original comments, but could say

Formally, project IDs must contain 6-63 lowercase letters, ...

then

This regex is used for for basic parsing of table references rather than validation purpose, e.g. it allows looser restriction for testing on mock resources. It may allow for patterns that would be rejected by the service

@kberezin-nshl
Copy link
Contributor Author

Thanks @Abacn , I have adjusted the comment accordingly.

*/
private static final String PROJECT_ID_REGEXP = "[a-z][-a-z0-9:.]{4,61}[a-z0-9]";
private static final String PROJECT_ID_REGEXP = "[a-z][-a-z0-9:.]{0,62}";
Copy link
Contributor

@Abacn Abacn Aug 22, 2024

Choose a reason for hiding this comment

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

I thought further about it, previously project id cannot ends with colon and dot, now it does not guard against it. I cannot tell if this could have unexpected results. Consider use [a-z][-a-z0-9:.]{0,61}[a-z0-9] ? then effectively it still requires project id of at least 2 chars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, previously project ID couldn't have less than 5 symbols either 😄 I think that's the point of the change, enforcing somebody else's rules shouldn't be a concern of 3rd party library, which Apache Beam is.

Having said that, I made the requested change because I think "at least 2 chars long" is a reasonable restriction.

We really hope this change will make it into 2.59.0!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please have a look again, @Abacn ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It passed the 2.59.0 release cut unfortunately. You can try open a cherry pick PR to release-2.59.0 branch (similar to this one: #32290) and up to release manager cc: @lostluck available for cherry-pick

@Abacn Abacn merged commit b2ed1c5 into apache:master Aug 23, 2024
18 checks passed
kberezin-nshl added a commit to nutshelllabs/beam that referenced this pull request Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants