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: Remove all client side validation for OLM, allow nonspecific lif… #1160

Merged
merged 7 commits into from Jan 7, 2022

Conversation

JesseLovelace
Copy link
Contributor

Removes client side validation by refactoring LifecycleAction into a non-abstract class and allowing for nonspecific lifecycle actions.

@JesseLovelace JesseLovelace requested a review from a team as a code owner December 1, 2021 22:00
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 1, 2021
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Dec 1, 2021
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Thanks Jesse

* expressed as subclasses of this class, accessed by static factory methods.
*/
public abstract static class LifecycleAction implements Serializable {
public static class LifecycleAction implements Serializable {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to remove the modifier abstract in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Abstract classes can't be instantiated so we do if we want to allow for unsupported action types to be created in this way.

The alternative would be to make an implementation of LifecycleAction for that purpose, like "CustomLifecycleAction" or something, but I'm worried that doing that would be confusing, or imply that this is the correct way to use action types that aren't in the library (when in actuality users should upgrade to the latest version of the library)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, totally missed that. Thanks Jesse, I would confirm what happens when a say setStorageClass doesn't have storageClass in the rule (the case an action type is added that requires additional parameters) and see how GCS handles the request on a PATCH request.

I suspect it will fail; so we may want to clarify if we want the library not to create an instance for that unknown action type.

@@ -548,8 +548,12 @@ static LifecycleRule fromPb(Rule rule) {
StorageClass.valueOf(action.getStorageClass()));
break;
default:
throw new UnsupportedOperationException(
"The specified lifecycle action " + action.getType() + " is not currently supported");
log.warning(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add tests to ensure unknown types no longer throw exceptions?

@frankyn
Copy link
Member

frankyn commented Dec 14, 2021

@JesseLovelace are you still addressing the clirr issue?

public abstract String getActionType();
private final String actionType;

private LifecycleAction(String actionType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make this constructor public to allow someone (as remote a chance as it is) that is already extending themselves allow them to continue to extend, but now they would need to provide the actionType.

@JesseLovelace JesseLovelace requested a review from a team as a code owner January 5, 2022 23:15
@JesseLovelace JesseLovelace merged commit 5a160ee into main Jan 7, 2022
@JesseLovelace JesseLovelace deleted the fixolmaction branch January 7, 2022 23:47
@frankyn
Copy link
Member

frankyn commented Feb 11, 2022

/cherry-pick v2.1.9

@frankyn
Copy link
Member

frankyn commented Feb 11, 2022

/cherry-pick 2.1.x

frankyn pushed a commit that referenced this pull request Feb 15, 2022
#1160)

* fix: Remove all client side validation for OLM, allow nonspecific lifecycle actions

* Test unsupported actions

* address comments

* Fix clirr

* 🦉 Updates from OwlBot

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* checkstyle

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
@frankyn frankyn mentioned this pull request Feb 15, 2022
frankyn added a commit that referenced this pull request Feb 15, 2022
* fix: Remove all client side validation for OLM, allow nonspecific lifecycle actions

* Test unsupported actions

* address comments

* Fix clirr

* 🦉 Updates from OwlBot

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* checkstyle

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

Co-authored-by: JesseLovelace <43148100+JesseLovelace@users.noreply.github.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants