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.

Sign up for GitHub

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

Review #1327

Merged
merged 4 commits into from
Nov 15, 2016
Merged

Review #1327

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
281cd00
[NCL-2396]: Review database model - model part updated
jbartece Nov 11, 2016
02a724f
[NCL-2396]: Review database model - other layers
jbartece Nov 11, 2016
08b0e2f
Change unknown e-mail
jbartece Nov 14, 2016
d742ce2
Fix tests to insert user with correct e-mail to database
jbartece Nov 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion auth/src/main/java/org/jboss/pnc/auth/JAASLoggedInUser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public JAASLoggedInUser(HttpServletRequest httpServletRequest) {

@Override
public String getEmail() {
return "Email N/A (" + getUserName() + ")";
return "example@example.com";
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this class is currently not used in our production deployment it can be (generally looking, it is not for test only). The previous return string is better fit than hard-coded example.com.
You probably changed it due to email validation constraint. I see two options here:
a) update the class to read the email and other missing data from the User entity, in this case it would mean, if you use JAAS you should have another way to register new users (For tests we can use demo-data).
b) remove the constraint

Copy link
Contributor Author

@jbartece jbartece Nov 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I changed it due to the constraint and removing it is not the right way, I think.

I don't understand the A):
How can I read it from the User entity, when the entity is meant to be created based on what's obtained from the JAASAuthenticator?

Currently there is no e-mail written to the DB, so I can change it to smth like:
getUserName() + "@example.com"
or
getUserName() + "@unknown.com"
to preserve the information there and to have the correct e-mail address format.

Copy link
Contributor

Choose a reason for hiding this comment

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

User entity inserted only when the user does not exists. We have "auto-registration" for authenticated users. You can insert a user with proper email and matching username, before the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. That could be done in the test probably, but why not to change it to return a valid e-mail?

}

@Override
Expand Down
32 changes: 21 additions & 11 deletions datastore/src/test/java/org/jboss/pnc/datastore/DatastoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,7 @@
import org.jboss.arquillian.junit.Arquillian;
import org.jboss.arquillian.junit.InSequence;
import org.jboss.arquillian.transaction.api.annotation.Transactional;
import org.jboss.pnc.model.Artifact;
import org.jboss.pnc.model.ArtifactRepo;
import org.jboss.pnc.model.BuildConfiguration;
import org.jboss.pnc.model.BuildConfigurationAudited;
import org.jboss.pnc.model.BuildEnvironment;
import org.jboss.pnc.model.BuildRecord;
import org.jboss.pnc.model.License;
import org.jboss.pnc.model.Project;
import org.jboss.pnc.model.SystemImageType;
import org.jboss.pnc.model.*;
import org.jboss.pnc.spi.datastore.Datastore;
import org.jboss.pnc.spi.datastore.repositories.ArtifactRepository;
import org.jboss.pnc.spi.datastore.repositories.BuildConfigurationAuditedRepository;
Expand All @@ -39,6 +31,7 @@
import org.jboss.pnc.spi.datastore.repositories.LicenseRepository;
import org.jboss.pnc.spi.datastore.repositories.ProductRepository;
import org.jboss.pnc.spi.datastore.repositories.ProjectRepository;
import org.jboss.pnc.spi.datastore.repositories.UserRepository;
import org.jboss.pnc.test.category.ContainerTest;
import org.jboss.shrinkwrap.api.Archive;
import org.junit.Assert;
Expand All @@ -47,6 +40,7 @@
import org.junit.runner.RunWith;

import javax.inject.Inject;

import java.time.Instant;
import java.util.Date;
import java.util.List;
Expand Down Expand Up @@ -97,6 +91,9 @@ public class DatastoreTest {

@Inject
LicenseRepository licenseRepository;

@Inject
UserRepository userRepository;

@Inject
Datastore datastore;
Expand Down Expand Up @@ -162,10 +159,16 @@ public void initBuildRecordData() throws Exception {
.importDate(Date.from(Instant.now()))
.repoType(ArtifactRepo.Type.MAVEN).build();

User user = User.Builder.newBuilder()
.username("pnc").email("pnc@redhat.com").build();
user = userRepository.save(user);
Assert.assertNotNull(user.getId());

BuildRecord buildRecord = BuildRecord.Builder.newBuilder().id(datastore.getNextBuildRecordId())
.buildConfigurationAudited(buildConfigAud).latestBuildConfiguration(buildConfig)
.submitTime(Date.from(Instant.now())).startTime(Date.from(Instant.now())).endTime(Date.from(Instant.now()))
.builtArtifact(builtArtifact1).dependency(importedArtifact2).build();
.builtArtifact(builtArtifact1).dependency(importedArtifact2)
.user(user).build();

builtArtifact1 = artifactRepository.save(builtArtifact1);
importedArtifact2 = artifactRepository.save(importedArtifact2);
Expand Down Expand Up @@ -217,10 +220,17 @@ public void testDatastore() throws Exception {
.size(ARTIFACT_3_SIZE).originUrl("http://test/importArtifact2.jar")
.importDate(Date.from(Instant.now())).repoType(ArtifactRepo.Type.MAVEN).build();


User user = User.Builder.newBuilder()
.username("pnc2").email("pnc2@redhat.com").build();
user = userRepository.save(user);
Assert.assertNotNull(user.getId());

BuildRecord.Builder buildRecordBuilder = BuildRecord.Builder.newBuilder().id(datastore.getNextBuildRecordId())
.buildConfigurationAudited(buildConfigAud).latestBuildConfiguration(buildConfig)
.submitTime(Date.from(Instant.now())).startTime(Date.from(Instant.now())).endTime(Date.from(Instant.now()))
.builtArtifact(builtArtifact1).dependency(importedArtifact2).builtArtifact(builtArtifact3);
.builtArtifact(builtArtifact1).dependency(importedArtifact2).builtArtifact(builtArtifact3)
.user(user);

BuildRecord buildRecord = datastore.storeCompletedBuild(buildRecordBuilder);

Expand Down
4 changes: 0 additions & 4 deletions integration-test/src/test/java/org/jboss/pnc/integration/BuildConfigurationRestTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,6 @@ public void shouldUpdateBuildConfiguration() throws IOException {
configurationTemplate.addValue("_scmRepoURL", PNC_REPO);
configurationTemplate.addValue("_creationTime", String.valueOf(1518382545038L));
configurationTemplate.addValue("_lastModificationTime", String.valueOf(155382545038L));
configurationTemplate.addValue("_repositories", "");
configurationTemplate.addValue("_projectId", updatedProjectId);
configurationTemplate.addValue("_environmentId", String.valueOf(environmentId));
configurationTemplate.addValue("_genParamValue1", updatedGenParamValue);
Expand Down Expand Up @@ -284,8 +283,6 @@ public void shouldCloneBuildConfiguration() {
.isEqualTo(clonedBuildConfiguration.body().jsonPath().getString("content.buildScript"));
assertThat(originalBuildConfiguration.body().jsonPath().getString("content.scmRepoURL"))
.isEqualTo(clonedBuildConfiguration.body().jsonPath().getString("content.scmRepoURL"));
assertThat(originalBuildConfiguration.body().jsonPath().getString("content.repositories"))
.isEqualTo(clonedBuildConfiguration.body().jsonPath().getString("content.repositories"));
assertTrue(originalBuildConfiguration.body().jsonPath().getString("content.genericParameters.KEY1")
.equals(clonedBuildConfiguration.body().jsonPath().getString("content.genericParameters.KEY1")));
}
Expand All @@ -300,7 +297,6 @@ public void shouldFailToCreateNewBuildConfigurationBecauseIdIsNotNull() throws I
configurationTemplate.addValue("_environmentId", String.valueOf(environmentId));
configurationTemplate.addValue("_creationTime", String.valueOf(1518382545038L));
configurationTemplate.addValue("_lastModificationTime", String.valueOf(155382545038L));
configurationTemplate.addValue("_repositories", "");

given().headers(testHeaders)
.body(configurationTemplate.fillTemplate()).contentType(ContentType.JSON).port(getHttpPort()).when()
Expand Down
1 change: 0 additions & 1 deletion integration-test/src/test/resources/buildConfiguration_create_template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"scmRepoURL":"https://github.com/project-ncl/pnc.git",
"creationTime":1418382545021,
"lastModificationTime":1418382545038,
"repositories":null,
"project": {
"id": ${_projectId}
},
Expand Down
1 change: 0 additions & 1 deletion integration-test/src/test/resources/buildConfiguration_update_template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"scmRepoURL": "${_scmRepoURL}",
"creationTime": ${_creationTime},
"lastModificationTime": ${_lastModificationTime},
"repositories": "${_repositories}",
"project": {
"id": ${_projectId}
},
Expand Down
1 change: 0 additions & 1 deletion integration-test/src/test/resources/buildConfiguration_with_id_template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
"scmRepoURL": "${_scmRepoURL}",
"creationTime": ${_creationTime},
"lastModificationTime": ${_lastModificationTime},
"repositories": "${_repositories}",
"project": {
"id": ${_projectId}
},
Expand Down
2 changes: 1 addition & 1 deletion model/src/main/java/org/jboss/pnc/model/BuildConfigSetRecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public class BuildConfigSetRecord implements GenericEntity<Integer> {
/**
* The user who executed the set.
*/
// @NotNull //TODO uncomment
@NotNull
@ManyToOne
@ForeignKey(name = "fk_buildconfigsetrecord_user")
@Index(name="idx_buildconfigsetrecord_user")
Expand Down
28 changes: 0 additions & 28 deletions model/src/main/java/org/jboss/pnc/model/BuildConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,12 +188,6 @@ public class BuildConfiguration implements GenericEntity<Integer>, Cloneable {
@NotAudited
@ManyToMany(mappedBy = "dependencies")
private Set<BuildConfiguration> dependants;

// TODO: What data format does Aprox need?
// [jdcasey] I'm not sure what this is supposed to do in the repository
// manager...so hard to say what format is required.
// @Column(name = "repositories")
private String repositories;

@Getter
@Setter
Expand Down Expand Up @@ -528,20 +522,6 @@ public void setArchived(boolean archived) {
this.active = archived ? null : true;
}

/**
* @return the repositories
*/
public String getRepositories() {
return repositories;
}

/**
* @param repositories the repositories to set
*/
public void setRepositories(String repositories) {
this.repositories = repositories;
}

public Set<BuildRecord> getBuildRecords() {
return buildRecords;
}
Expand Down Expand Up @@ -710,8 +690,6 @@ public static class Builder {
private Date lastModificationTime;

private boolean archived = false;

private String repositories;

private Map<String, String> genericParameters = new HashMap<>();

Expand Down Expand Up @@ -746,7 +724,6 @@ public BuildConfiguration build() {
buildConfiguration.setCreationTime(creationTime);
buildConfiguration.setLastModificationTime(lastModificationTime);
buildConfiguration.setArchived(archived);
buildConfiguration.setRepositories(repositories);
buildConfiguration.setGenericParameters(genericParameters);
buildConfiguration.setBuildConfigurationSets(buildConfigurationSets);
buildConfiguration.setProductVersion(productVersion);
Expand Down Expand Up @@ -861,11 +838,6 @@ public Builder archived(boolean archived) {
this.archived = archived;
return this;
}

public Builder repositories(String repositories) {
this.repositories = repositories;
return this;
}

public Builder genericParameters(Map<String, String> genericParameters) {
this.genericParameters = genericParameters;
Expand Down
10 changes: 5 additions & 5 deletions model/src/main/java/org/jboss/pnc/model/BuildRecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ public class BuildRecord implements GenericEntity<Integer> {
@Column(columnDefinition="timestamp with time zone")
private Date endTime;

// @NotNull //TODO uncomment
@NotNull
@ManyToOne
@ForeignKey(name = "fk_buildrecord_user")
@Index(name="idx_buildrecord_user")
Expand Down Expand Up @@ -158,10 +158,12 @@ public class BuildRecord implements GenericEntity<Integer> {

@Getter
@Setter
@Size(max=150)
private String sshCommand;

@Getter
@Setter
@Size(max=64)
private String sshPassword;

/**
Expand All @@ -172,6 +174,7 @@ public class BuildRecord implements GenericEntity<Integer> {
*/
@Getter
@Setter
@Size(max=255)
private String executionRootName;

/**
Expand All @@ -180,12 +183,9 @@ public class BuildRecord implements GenericEntity<Integer> {
*/
@Getter
@Setter
@Size(max=100)
private String executionRootVersion;

private boolean tested;

private boolean deprecated;

/**
* Artifacts which were produced by this build
*/
Expand Down
2 changes: 1 addition & 1 deletion model/src/main/java/org/jboss/pnc/model/License.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public class License implements GenericEntity<Integer> {
@Size(max=255)
private String refUrl;

@Size(max=255)
@Size(max=20)
private String shortName;

/**
Expand Down
2 changes: 1 addition & 1 deletion model/src/main/java/org/jboss/pnc/model/ProductMilestone.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public class ProductMilestone implements GenericEntity<Integer> {
*/
@Pattern(message="The version should consist of three numeric parts and one alphanumeric qualifier each separated by a dot" , regexp="^[0-9]+\\.[0-9]+\\.[0-9]+\\.[\\w]+$")
@NotNull
@Size(max=255)
@Size(max=50)
private String version;

/**
Expand Down
2 changes: 2 additions & 0 deletions model/src/main/java/org/jboss/pnc/model/ProductMilestoneRelease.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,10 @@ public class ProductMilestoneRelease implements GenericEntity<Integer> {
@ForeignKey(name = "fk_productmilestone_milestonerelease")
@JoinColumn(updatable = false)
private ProductMilestone milestone;

@Enumerated(EnumType.STRING)
private MilestoneReleaseStatus status;

@Lob
@Type(type = "org.hibernate.type.TextType")
private String log;
Expand Down
2 changes: 1 addition & 1 deletion model/src/main/java/org/jboss/pnc/model/ProductRelease.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public class ProductRelease implements GenericEntity<Integer> {
*/
@Pattern(message="The version should consist of three numeric parts and one alphanumeric qualifier each separated by a dot" , regexp="^[0-9]+\\.[0-9]+\\.[0-9]+\\.[\\w]+$")
@NotNull
@Size(max=255)
@Size(max=50)
private String version;

@Enumerated(EnumType.STRING)
Expand Down
6 changes: 5 additions & 1 deletion model/src/main/java/org/jboss/pnc/model/User.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@
*/
package org.jboss.pnc.model;

import org.hibernate.validator.constraints.Email;

import javax.persistence.*;
import javax.validation.constraints.NotNull;

import java.util.ArrayList;
import java.util.List;

Expand All @@ -31,7 +34,7 @@
* @author avibelli
*/
@Entity
@Table(name = "Users", uniqueConstraints = { @UniqueConstraint(name = "uk_user_email", columnNames = { "email" }),
@Table(uniqueConstraints = { @UniqueConstraint(name = "uk_user_email", columnNames = { "email" }),
@UniqueConstraint(name = "uk_user_username", columnNames = { "username" }) })
public class User implements GenericEntity<Integer> {

Expand All @@ -47,6 +50,7 @@ public class User implements GenericEntity<Integer> {

@Column(unique = true)
@NotNull
@Email
@Size(max=255)
private String email;

Expand Down
1 change: 1 addition & 0 deletions model/src/test/java/org/jboss/pnc/model/AbstractModelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ protected static void clearDatabaseTables() {
em.createNativeQuery("delete from ProductRelease").executeUpdate();
em.createNativeQuery("delete from ProductVersion").executeUpdate();
em.createNativeQuery("delete from Project").executeUpdate();
em.createNativeQuery("delete from User").executeUpdate();
em.createNativeQuery("SET DATABASE REFERENTIAL INTEGRITY TRUE").executeUpdate();
tx.commit();

Expand Down
8 changes: 7 additions & 1 deletion model/src/test/java/org/jboss/pnc/model/BasicModelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ public class BasicModelTest extends AbstractModelTest {

/** located in src/test/resources */
private final static String DBUNIT_DATASET_FILE = "basic-model-test-data.xml";

private User pncUser;

/**
* Initialize a basic data set before each test run
Expand All @@ -60,6 +62,8 @@ public void initTestData() throws Exception {
em.persist(buildConfig2);
em.getTransaction().commit();
em.close();

this.pncUser = User.Builder.newBuilder().id(1).build();
}

@After
Expand Down Expand Up @@ -136,7 +140,8 @@ public void testCreateBuildRecordAndArtifacts() {
BuildRecord buildRecord1 = BuildRecord.Builder.newBuilder().id(1).buildConfigurationAudited(buildConfigAud)
.latestBuildConfiguration(buildConfig1).buildLog("Bulid Complete").buildContentId("foo")
.submitTime(Date.from(Instant.now())).startTime(Date.from(Instant.now())).endTime(Date.from(Instant.now()))
.builtArtifact(artifact1).builtArtifact(artifact2).dependency(artifact3).build();
.builtArtifact(artifact1).builtArtifact(artifact2).dependency(artifact3)
.user(pncUser).build();

em.getTransaction().begin();
em.persist(artifact1);
Expand Down Expand Up @@ -173,6 +178,7 @@ public void testBuildRecordPreventsAddingDuplicateArtifacts() {
.submitTime(Date.from(Instant.now())).startTime(Date.from(Instant.now())).endTime(Date.from(Instant.now()))
//Add the built artifact and dependency artifact twice
.builtArtifact(builtArtifact).builtArtifact(builtArtifact).dependency(importedArtifact).dependency(importedArtifact)
.user(pncUser)
.build();

em.getTransaction().begin();
Expand Down
2 changes: 2 additions & 0 deletions model/src/test/resources/basic-model-test-data.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,6 @@
<project id="2" name="Test Project 2" description="Test Project 2 Description"
issueTrackerUrl="http://issues.jboss.org" license_id="2" />

<user id="1" email="pnc@redhat.com" username="pnc" />

</dataset>
Loading