From b0242161076f6d372ab689b2db5566223e31b172 Mon Sep 17 00:00:00 2001 From: Chanseok Oh Date: Wed, 15 Feb 2017 17:43:33 -0500 Subject: [PATCH] Validate Project Selector (#1410) * Validate Project Selector * Create and share one IValidator instance. * Remove ununsed Import-Package * Clean up code * checkArgument (checkState x) / simplify validator * Remove hamcrest --- .../StandardDeployPreferencesPanelTest.java | 10 +-- .../deploy/ui/DeployPreferencesDialog.java | 8 +- .../ui/StandardDeployPreferencesPanel.java | 31 +++++-- .../projectselector/ProjectSelectorTest.java | 6 +- .../projectselector/ProjectSelector.java | 5 -- .../ProjectIdInputValidatorTest.java | 86 ------------------- .../ProjectSelectorValidatorTest.java | 35 ++++++++ .../META-INF/MANIFEST.MF | 1 - ...tor.java => ProjectSelectorValidator.java} | 27 +----- .../tools/eclipse/ui/util/messages.properties | 3 +- 10 files changed, 67 insertions(+), 145 deletions(-) delete mode 100644 plugins/com.google.cloud.tools.eclipse.ui.util.test/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectIdInputValidatorTest.java create mode 100644 plugins/com.google.cloud.tools.eclipse.ui.util.test/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectSelectorValidatorTest.java rename plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/databinding/{ProjectIdInputValidator.java => ProjectSelectorValidator.java} (54%) diff --git a/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui.test/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/StandardDeployPreferencesPanelTest.java b/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui.test/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/StandardDeployPreferencesPanelTest.java index 7bc756c588..5c13cf41de 100644 --- a/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui.test/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/StandardDeployPreferencesPanelTest.java +++ b/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui.test/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/StandardDeployPreferencesPanelTest.java @@ -17,7 +17,6 @@ package com.google.cloud.tools.eclipse.appengine.deploy.ui; import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -80,13 +79,6 @@ public void setUp() throws Exception { when(account2.getOAuth2Credential()).thenReturn(mock(Credential.class)); } - @Test - public void testHasSelection() { - StandardDeployPreferencesPanel deployPanel = new StandardDeployPreferencesPanel( - parent, project, loginService, layoutChangedHandler, true, projectRepository); - assertFalse(deployPanel.hasSelection()); - } - @Test public void testSelectSingleAccount() { when(loginService.getAccounts()).thenReturn(new HashSet<>(Arrays.asList(account1))); @@ -148,7 +140,7 @@ public void testProjectSavedInPreferencesSelected() throws ProjectRepositoryExce fail("Did not find ProjectSelector widget"); } - private void initializeProjectRepository(ProjectRepository projectRepository) + private void initializeProjectRepository(ProjectRepository projectRepository) throws ProjectRepositoryException { GcpProject project1 = new GcpProject("Project1", "projectId1"); GcpProject project2 = new GcpProject("Project2", "projectId2"); diff --git a/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/DeployPreferencesDialog.java b/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/DeployPreferencesDialog.java index 501647ae3e..28c91e1f53 100644 --- a/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/DeployPreferencesDialog.java +++ b/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/DeployPreferencesDialog.java @@ -76,9 +76,7 @@ protected Control createContents(Composite parent) { setTitleImage(titleImage); } - Button deployButton = getButton(IDialogConstants.OK_ID); - deployButton.setText(Messages.getString("deploy")); - deployButton.setEnabled(false); + getButton(IDialogConstants.OK_ID).setText(Messages.getString("deploy")); // TitleAreaDialogSupport does not validate initially, let's trigger validation this way content.getDataBindingContext().updateTargets(); @@ -109,7 +107,7 @@ protected Control createDialogArea(final Composite parent) { @Override public int getMessageType(ValidationStatusProvider statusProvider) { int type = super.getMessageType(statusProvider); - setValid(type != IMessageProvider.ERROR && content.hasSelection()); + setValid(type != IMessageProvider.ERROR); return type; } }); @@ -180,7 +178,7 @@ public boolean isHelpAvailable() { private void setValid(boolean isValid) { Button deployButton = getButton(IDialogConstants.OK_ID); if (deployButton != null) { - deployButton.setEnabled(isValid && content.hasSelection()); + deployButton.setEnabled(isValid); } } diff --git a/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/StandardDeployPreferencesPanel.java b/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/StandardDeployPreferencesPanel.java index f6a92d0e5c..b64c091140 100644 --- a/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/StandardDeployPreferencesPanel.java +++ b/plugins/com.google.cloud.tools.eclipse.appengine.deploy.ui/src/com/google/cloud/tools/eclipse/appengine/deploy/ui/StandardDeployPreferencesPanel.java @@ -20,15 +20,17 @@ import com.google.cloud.tools.eclipse.login.IGoogleLoginService; import com.google.cloud.tools.eclipse.login.ui.AccountSelector; import com.google.cloud.tools.eclipse.login.ui.AccountSelectorObservableValue; -import com.google.cloud.tools.eclipse.projectselector.ProjectRepository; import com.google.cloud.tools.eclipse.projectselector.GcpProject; +import com.google.cloud.tools.eclipse.projectselector.ProjectRepository; import com.google.cloud.tools.eclipse.projectselector.ProjectRepositoryException; import com.google.cloud.tools.eclipse.projectselector.ProjectSelector; import com.google.cloud.tools.eclipse.ui.util.FontUtil; import com.google.cloud.tools.eclipse.ui.util.databinding.BucketNameValidator; +import com.google.cloud.tools.eclipse.ui.util.databinding.ProjectSelectorValidator; import com.google.cloud.tools.eclipse.ui.util.databinding.ProjectVersionValidator; import com.google.cloud.tools.eclipse.util.status.StatusUtil; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import com.google.common.base.Strings; import java.util.Collections; import java.util.List; @@ -44,6 +46,7 @@ import org.eclipse.core.databinding.observable.list.IObservableList; import org.eclipse.core.databinding.observable.value.IObservableValue; import org.eclipse.core.databinding.observable.value.WritableValue; +import org.eclipse.core.databinding.validation.IValidator; import org.eclipse.core.databinding.validation.MultiValidator; import org.eclipse.core.databinding.validation.ValidationStatus; import org.eclipse.core.resources.IProject; @@ -181,11 +184,21 @@ public Object convert(Object expectedEmail) { } private void setupProjectIdDataBinding(DataBindingContext context) { - IViewerObservableValue projectList = ViewerProperties.singleSelection().observe(projectSelector.getViewer()); + IViewerObservableValue projectList = + ViewerProperties.singleSelection().observe(projectSelector.getViewer()); IObservableValue projectIdModel = PojoProperties.value("projectId").observe(model); - context.bindValue(projectList, projectIdModel, - new UpdateValueStrategy().setConverter(new GcpProjectToProjectIdConverter()), - new UpdateValueStrategy().setConverter(new ProjectIdToGcpProjectConverter())); + + UpdateValueStrategy gcpProjectToProjectId = + new UpdateValueStrategy().setConverter(new GcpProjectToProjectIdConverter()); + UpdateValueStrategy projectIdToGcpProject = + new UpdateValueStrategy().setConverter(new ProjectIdToGcpProjectConverter()); + if (requireValues) { + IValidator validator = new ProjectSelectorValidator(); + gcpProjectToProjectId.setAfterConvertValidator(validator); + projectIdToGcpProject.setAfterGetValidator(validator); + } + + context.bindValue(projectList, projectIdModel, gcpProjectToProjectId, projectIdToGcpProject); } private void setupProjectVersionDataBinding(DataBindingContext context) { @@ -386,6 +399,8 @@ public Object convert(Object fromObject) { if (fromObject == null) { return null; } + + Preconditions.checkArgument(fromObject instanceof String); try { return projectRepository.getProject(accountSelector.getSelectedCredential(), (String) fromObject); @@ -406,6 +421,8 @@ public Object convert(Object fromObject) { if (fromObject == null) { return null; } + + Preconditions.checkArgument(fromObject instanceof GcpProject); return ((GcpProject) fromObject).getId(); } } @@ -489,8 +506,4 @@ public void setFont(Font font) { expandableComposite.setFont(font); FontUtil.convertFontToBold(expandableComposite); } - - boolean hasSelection() { - return projectSelector.hasSelection(); - } } diff --git a/plugins/com.google.cloud.tools.eclipse.projectselector.test/src/com/google/cloud/tools/eclipse/projectselector/ProjectSelectorTest.java b/plugins/com.google.cloud.tools.eclipse.projectselector.test/src/com/google/cloud/tools/eclipse/projectselector/ProjectSelectorTest.java index 4add3ad932..e26f34258e 100644 --- a/plugins/com.google.cloud.tools.eclipse.projectselector.test/src/com/google/cloud/tools/eclipse/projectselector/ProjectSelectorTest.java +++ b/plugins/com.google.cloud.tools.eclipse.projectselector.test/src/com/google/cloud/tools/eclipse/projectselector/ProjectSelectorTest.java @@ -18,8 +18,6 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; import com.google.cloud.tools.eclipse.test.util.ui.ShellTestResource; import java.util.Arrays; @@ -64,16 +62,14 @@ public void testProjectsAreSortedAlphabetically() throws Exception { public void testSetProjectMaintainsSelection() { List projects = getUnsortedProjectList(); GcpProject selectedProject = projects.get(3); - + ProjectSelector projectSelector = new ProjectSelector(shellResource.getShell()); projectSelector.setProjects(projects); - assertFalse(projectSelector.hasSelection()); projectSelector.getViewer().setSelection(new StructuredSelection(selectedProject)); projectSelector.setProjects(projects.subList(2, projects.size())); IStructuredSelection selection = projectSelector.getViewer().getStructuredSelection(); assertThat(selection.size(), is(1)); - assertTrue(projectSelector.hasSelection()); assertThat((GcpProject) selection.getFirstElement(), is(selectedProject)); } diff --git a/plugins/com.google.cloud.tools.eclipse.projectselector/src/com/google/cloud/tools/eclipse/projectselector/ProjectSelector.java b/plugins/com.google.cloud.tools.eclipse.projectselector/src/com/google/cloud/tools/eclipse/projectselector/ProjectSelector.java index 3e12714051..84371a2a34 100644 --- a/plugins/com.google.cloud.tools.eclipse.projectselector/src/com/google/cloud/tools/eclipse/projectselector/ProjectSelector.java +++ b/plugins/com.google.cloud.tools.eclipse.projectselector/src/com/google/cloud/tools/eclipse/projectselector/ProjectSelector.java @@ -73,9 +73,4 @@ public void setProjects(List projects) { } tableViewer.setSelection(selection); } - - public boolean hasSelection() { - ISelection selection = tableViewer.getSelection(); - return !selection.isEmpty(); - } } diff --git a/plugins/com.google.cloud.tools.eclipse.ui.util.test/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectIdInputValidatorTest.java b/plugins/com.google.cloud.tools.eclipse.ui.util.test/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectIdInputValidatorTest.java deleted file mode 100644 index 95c9aa1d62..0000000000 --- a/plugins/com.google.cloud.tools.eclipse.ui.util.test/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectIdInputValidatorTest.java +++ /dev/null @@ -1,86 +0,0 @@ -/* - * Copyright 2016 Google Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.google.cloud.tools.eclipse.ui.util.databinding; - -import static org.hamcrest.CoreMatchers.is; -import static org.junit.Assert.assertThat; - -import org.eclipse.core.runtime.IStatus; -import org.junit.Test; - -public class ProjectIdInputValidatorTest { - - @Test - public void testValidate_nonStringInput() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate(new Object()).getSeverity(), is(IStatus.ERROR)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate(new Object()).getSeverity(), is(IStatus.ERROR)); - } - - @Test - public void testValidate_emptyString() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate("").getSeverity(),is(IStatus.OK)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate("").getSeverity(),is(IStatus.ERROR)); - } - - @Test - public void testValidate_upperCaseLetter() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate("asdfghijK").getSeverity(), is(IStatus.OK)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate("asdfghijK").getSeverity(), is(IStatus.OK)); - } - - @Test - public void testValidate_startWithNumber() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate("1asdfghij").getSeverity(), is(IStatus.OK)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate("1asdfghij").getSeverity(), is(IStatus.OK)); - } - - @Test - public void testValidate_startWithHyphen() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate("-asdfghij").getSeverity(), is(IStatus.OK)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate("-asdfghij").getSeverity(), is(IStatus.OK)); - } - - @Test - public void testValidate_endWithHyphen() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate("asdfghij-").getSeverity(), is(IStatus.OK)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate("asdfghij-").getSeverity(), is(IStatus.OK)); - } - - @Test - public void testValidate_validName() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate("asdf-1ghij-2").getSeverity(), is(IStatus.OK)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate("asdf-1ghij-2").getSeverity(), is(IStatus.OK)); - } - - @Test - public void testValidate_maxLengthName() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate("a23456789012345678901234567890").getSeverity(), is(IStatus.OK)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate("a23456789012345678901234567890").getSeverity(), is(IStatus.OK)); - } - - @Test - public void testValidate_tooLongName() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate("a234567890123456789012345678901").getSeverity(), is(IStatus.OK)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate("a234567890123456789012345678901").getSeverity(), is(IStatus.OK)); - } - - @Test - public void testValidate_tooShortName() { - assertThat(new ProjectIdInputValidator(false /* requireProjectId */).validate("a2345").getSeverity(), is(IStatus.OK)); - assertThat(new ProjectIdInputValidator(true /* requireProjectId */).validate("a2345").getSeverity(), is(IStatus.OK)); - } -} diff --git a/plugins/com.google.cloud.tools.eclipse.ui.util.test/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectSelectorValidatorTest.java b/plugins/com.google.cloud.tools.eclipse.ui.util.test/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectSelectorValidatorTest.java new file mode 100644 index 0000000000..2348da4b4b --- /dev/null +++ b/plugins/com.google.cloud.tools.eclipse.ui.util.test/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectSelectorValidatorTest.java @@ -0,0 +1,35 @@ +/* + * Copyright 2017 Google Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.cloud.tools.eclipse.ui.util.databinding; + +import static org.junit.Assert.assertEquals; + +import org.eclipse.core.runtime.IStatus; +import org.junit.Test; + +public class ProjectSelectorValidatorTest { + + @Test + public void testValidate_nullString() { + assertEquals(IStatus.ERROR, new ProjectSelectorValidator().validate(null).getSeverity()); + } + + @Test + public void testValidate_emptyString() { + assertEquals(IStatus.ERROR, new ProjectSelectorValidator().validate("").getSeverity()); + } +} diff --git a/plugins/com.google.cloud.tools.eclipse.ui.util/META-INF/MANIFEST.MF b/plugins/com.google.cloud.tools.eclipse.ui.util/META-INF/MANIFEST.MF index d8cb721331..c95c673411 100644 --- a/plugins/com.google.cloud.tools.eclipse.ui.util/META-INF/MANIFEST.MF +++ b/plugins/com.google.cloud.tools.eclipse.ui.util/META-INF/MANIFEST.MF @@ -13,7 +13,6 @@ Require-Bundle: org.eclipse.ui.workbench, Import-Package: com.google.cloud.tools.eclipse.ui.util, com.google.cloud.tools.eclipse.util, com.google.cloud.tools.eclipse.util.status, - com.google.cloud.tools.project;version="0.1.9", com.google.common.annotations;version="[20.0.0,21.0.0)", com.google.common.base;version="[20.0.0,21.0.0)", org.eclipse.core.commands, diff --git a/plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectIdInputValidator.java b/plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectSelectorValidator.java similarity index 54% rename from plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectIdInputValidator.java rename to plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectSelectorValidator.java index 4ea1818425..f835f70341 100644 --- a/plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectIdInputValidator.java +++ b/plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/databinding/ProjectSelectorValidator.java @@ -17,36 +17,17 @@ package com.google.cloud.tools.eclipse.ui.util.databinding; import com.google.cloud.tools.eclipse.ui.util.Messages; -import com.google.cloud.tools.project.ProjectIdValidator; import org.eclipse.core.databinding.validation.IValidator; import org.eclipse.core.databinding.validation.ValidationStatus; import org.eclipse.core.runtime.IStatus; -public class ProjectIdInputValidator implements IValidator { - private boolean requireProjectId = true; - - public ProjectIdInputValidator(boolean requireProjectId) { - this.requireProjectId = requireProjectId; - } +public class ProjectSelectorValidator implements IValidator { @Override public IStatus validate(Object input) { - if (!(input instanceof String)) { - return ValidationStatus.error(Messages.getString("project.id.invalid")); //$NON-NLS-1$ - } - String value = (String) input; - return validateString(value); - } - - private IStatus validateString(String value) { - if (value.isEmpty()) { - return requireProjectId ? - ValidationStatus.error(Messages.getString("project.id.empty")) : //$NON-NLS-1$ - ValidationStatus.ok(); - } else if (ProjectIdValidator.validate(value)) { - return ValidationStatus.ok(); - } else { - return ValidationStatus.error(Messages.getString("project.id.invalid")); //$NON-NLS-1$ + if (input == null || ((String) input).isEmpty()) { + return ValidationStatus.error(Messages.getString("project.not.selected")); //$NON-NLS-1$ } + return ValidationStatus.ok(); } } diff --git a/plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/messages.properties b/plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/messages.properties index ffd64bca8d..3623c6ca5b 100644 --- a/plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/messages.properties +++ b/plugins/com.google.cloud.tools.eclipse.ui.util/src/com/google/cloud/tools/eclipse/ui/util/messages.properties @@ -1,5 +1,4 @@ bucket.name.invalid=Invalid bucket name -project.id.invalid=Invalid project ID -project.id.empty=Empty project ID +project.not.selected=Project ID not selected version.invalid=Version must contain only lower-case letters, numbers, and hyphens version.reserved=Version uses reserved form \ No newline at end of file