Skip to content

Commit

Permalink
Backport fixes (Apicurio#3966)
Browse files Browse the repository at this point in the history 
* Add null checks on  role mapping (Apicurio#3800)

* Add null checks on  role mapping

* Add check for Role and Unit Test

* Fixed a regression in the Settings page search box (Apicurio#3798)

* modified state change checks in ArtifactStateExt and AbstractSqlRegistryStorage (Apicurio#3637)

* Incorrect default value avro serialization (Apicurio#3633)

* Fix default value converter

* Add test for default byte type value in converter test

* Correct comment

* added unit tests (Apicurio#3593)

* Bugfix: prase decimal's default

---------

Co-authored-by: Amoncy <[email protected]>
Co-authored-by: Eric Wittmann <[email protected]>
Co-authored-by: Jean Xiao <[email protected]>
Co-authored-by: Anshu Anna Moncy <[email protected]>
Co-authored-by: j2gg0s <[email protected]>
  • Loading branch information
@carlesarnal @Amoncy
@EricWittmann @renjingxiao @j2gg0s
6 people authored Nov 13, 2023
1 parent 838249e commit cd4b580
Show file tree
Hide file tree
Showing 12 changed files with 1,433 additions and 402 deletions.
15 changes: 15 additions & 0 deletions app/src/main/java/io/apicurio/registry/rest/v2/AdminResourceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,12 @@ public class AdminResourceImpl implements AdminResource {
@Info(category = "download", description = "Download link expiry", availableSince = "2.1.2.Final")
Supplier<Long> downloadHrefTtl;

private static void requireParameter(String parameterName, Object parameterValue) {
if (parameterValue == null) {
throw new MissingRequiredParameterException(parameterName);
}
}

/**
* @see io.apicurio.registry.rest.v2.AdminResource#listArtifactTypes()
*/
Expand Down Expand Up @@ -173,6 +179,13 @@ public List<RuleType> listGlobalRules() {
@Audited(extractParameters = {"0", KEY_RULE})
@Authorized(style=AuthorizedStyle.None, level=AuthorizedLevel.Admin)
public void createGlobalRule(Rule data) {
RuleType type = data.getType();
requireParameter("type", type);

if (data.getConfig() == null || data.getConfig().isEmpty()) {
throw new MissingRequiredParameterException("Config");
}

RuleConfigurationDto configDto = new RuleConfigurationDto();
configDto.setConfiguration(data.getConfig());
storage.createGlobalRule(data.getType(), configDto);
Expand Down Expand Up @@ -391,6 +404,8 @@ public RoleMapping getRoleMapping(String principalId) {
@Authorized(style=AuthorizedStyle.None, level=AuthorizedLevel.Admin)
@RoleBasedAccessApiOperation
public void updateRoleMapping(String principalId, UpdateRole data) {
requireParameter("principalId", principalId);
requireParameter("role", data.getRole());
storage.updateRoleMapping(principalId, data.getRole().name());
}

Expand Down
5 changes: 5 additions & 0 deletions app/src/main/java/io/apicurio/registry/rest/v2/GroupsResourceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,11 @@ public ArtifactOwner getArtifactOwner(String groupId, String artifactId) {
public void updateArtifactOwner(String groupId, String artifactId, ArtifactOwner data) {
requireParameter("groupId", groupId);
requireParameter("artifactId", artifactId);
requireParameter("data", data);

if (data.getOwner().isEmpty()) {
throw new MissingRequiredParameterException("Missing required owner");
}

ArtifactOwnerDto dto = new ArtifactOwnerDto(data.getOwner());
storage.updateArtifactOwner(defaultGroupIdToNull(groupId), artifactId, dto);
Expand Down
14 changes: 8 additions & 6 deletions app/src/main/java/io/apicurio/registry/storage/ArtifactStateExt.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,16 @@ public void logIfDeprecated(String groupId, Object artifactId, Object version, A
}

public void applyState(Consumer<ArtifactState> consumer, ArtifactState previousState, ArtifactState newState) {
if (previousState != null) {
if (canTransition(previousState, newState)) {
consumer.accept(newState);
if ( previousState != newState) {
if (previousState != null) {
if (canTransition(previousState, newState)) {
consumer.accept(newState);
} else {
throw new InvalidArtifactStateException(previousState, newState);
}
} else {
throw new InvalidArtifactStateException(previousState, newState);
consumer.accept(newState);
}
} else {
consumer.accept(newState);
}
}
}
45 changes: 22 additions & 23 deletions app/src/main/java/io/apicurio/registry/storage/impl/sql/AbstractSqlRegistryStorage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -532,24 +532,23 @@ public void updateArtifactState(String groupId, String artifactId, ArtifactState
public void updateArtifactState(String groupId, String artifactId, String version, ArtifactState state)
throws ArtifactNotFoundException, VersionNotFoundException, RegistryStorageException {
log.debug("Updating the state of artifact {} {}, version {} to {}", groupId, artifactId, version, state.name());
ArtifactVersionMetaDataDto dto = this.getArtifactVersionMetaData(groupId, artifactId, version);
var metadata = getArtifactVersionMetaData(groupId, artifactId, version);
updateArtifactVersionStateRaw(metadata.getGlobalId(), metadata.getState(), state);
}


/**
* IMPORTANT: Private methods can't be @Transactional. Callers MUST have started a transaction.
*/
private void updateArtifactVersionStateRaw(long globalId, ArtifactState oldState, ArtifactState newState) {
handles.withHandleNoException(handle -> {
long globalId = dto.getGlobalId();
ArtifactState oldState = dto.getState();
ArtifactState newState = state;
if (oldState != newState) {
artifactStateEx.applyState(s -> {
String sql = sqlStatements.updateArtifactVersionState();
int rowCount = handle.createUpdate(sql)
.bind(0, s.name())
.bind(1, tenantContext.tenantId())
.bind(2, globalId)
.execute();
if (rowCount == 0) {
throw new VersionNotFoundException(groupId, artifactId, dto.getVersion());
}
}, oldState, newState);
}
artifactStateEx.applyState(s -> {
handle.createUpdate(sqlStatements.updateArtifactVersionState())
.bind(0, s.name())
.bind(1, tenantContext.tenantId())
.bind(2, globalId)
.execute();
}, oldState, newState);
return null;
});
}
Expand Down Expand Up @@ -2262,7 +2261,7 @@ protected CommentDto createArtifactVersionComment(String groupId, String artifac
ArtifactVersionMetaDataDto avmdd = res.orElseThrow(() -> new VersionNotFoundException(groupId, artifactId, version));

String cid = String.valueOf(commentId.generate(handle));

sql = sqlStatements.insertComment();
handle.createUpdate(sql)
.bind(0, tenantContext.tenantId())
Expand Down Expand Up @@ -2292,7 +2291,7 @@ protected CommentDto createArtifactVersionComment(String groupId, String artifac
throw new RegistryStorageException(e);
}
}

/**
* @see io.apicurio.registry.storage.RegistryStorage#getArtifactVersionComments(java.lang.String, java.lang.String, java.lang.String)
*/
Expand All @@ -2319,7 +2318,7 @@ public List<CommentDto> getArtifactVersionComments(String groupId, String artifa
throw new RegistryStorageException(e);
}
}

/**
* @see io.apicurio.registry.storage.RegistryStorage#deleteArtifactVersionComment(java.lang.String, java.lang.String, java.lang.String, java.lang.String)
*/
Expand Down Expand Up @@ -2360,7 +2359,7 @@ public void deleteArtifactVersionComment(String groupId, String artifactId, Stri
throw new RegistryStorageException(e);
}
}

/**
* @see io.apicurio.registry.storage.RegistryStorage#updateArtifactVersionComment(java.lang.String, java.lang.String, java.lang.String, java.lang.String, java.lang.String)
*/
Expand Down Expand Up @@ -3468,7 +3467,7 @@ public List<ArtifactReferenceDto> getInboundArtifactReferences(String groupId, S
.list();
});
}

/**
* @see RegistryStorage#isArtifactExists(String, String)
*/
Expand Down Expand Up @@ -3854,7 +3853,7 @@ protected void importGroup(Handle handle, GroupEntity entity) {
log.warn("Failed to import group entity (likely it already exists).", e);
}
}

protected void importComment(Handle handle, CommentEntity entity) {
try {
String sql = sqlStatements.insertComment();
Expand Down
125 changes: 125 additions & 0 deletions app/src/test/java/io/apicurio/registry/noprofile/rest/v2/GroupsResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,16 @@

import io.apicurio.registry.AbstractResourceTestBase;
import io.apicurio.registry.rest.client.exception.RuleViolationException;
import io.apicurio.registry.rest.v2.beans.ArtifactOwner;
import io.apicurio.registry.types.ArtifactState;
import io.apicurio.registry.rest.v2.beans.ArtifactMetaData;
import io.apicurio.registry.rest.v2.beans.ArtifactReference;
import io.apicurio.registry.rest.v2.beans.Comment;
import io.apicurio.registry.rest.v2.beans.EditableMetaData;
import io.apicurio.registry.rest.v2.beans.IfExists;
import io.apicurio.registry.rest.v2.beans.NewComment;
import io.apicurio.registry.rest.v2.beans.Rule;
import io.apicurio.registry.rest.v2.beans.UpdateState;
import io.apicurio.registry.rest.v2.beans.VersionMetaData;
import io.apicurio.registry.rules.compatibility.jsonschema.diff.DiffType;
import io.apicurio.registry.storage.impl.sql.SqlUtil;
Expand Down Expand Up @@ -178,6 +181,41 @@ public void testDefaultGroup() throws Exception {
.body("info.title", equalTo("Empty API"));
}

@Test
public void testUpdateArtifactOwner() throws Exception {
String oaiArtifactContent = resourceToString("openapi-empty.json");
createArtifact("testUpdateArtifactOwner", "testUpdateArtifactOwner/EmptyAPI/1",ArtifactType.OPENAPI, oaiArtifactContent);

ArtifactOwner artifactOwner = new ArtifactOwner("newOwner");

given()
.when()
.contentType(CT_JSON)
.pathParam("groupId", "testUpdateArtifactOwner")
.pathParam("artifactId", "testUpdateArtifactOwner/EmptyAPI/1")
.body(artifactOwner)
.put("/registry/v2/groups/{groupId}/artifacts/{artifactId}/owner")
.then()
.statusCode(204);
}

@Test
public void testUpdateEmptyArtifactOwner() throws Exception {
String oaiArtifactContent = resourceToString("openapi-empty.json");
createArtifact("testUpdateEmptyArtifactOwner", "testUpdateEmptyArtifactOwner/EmptyAPI/1",ArtifactType.OPENAPI, oaiArtifactContent);

ArtifactOwner artifactOwner = new ArtifactOwner("");

given()
.when()
.contentType(CT_JSON)
.pathParam("groupId", "testUpdateEmptyArtifactOwner")
.pathParam("artifactId", "testUpdateEmptyArtifactOwner/EmptyAPI/1")
.body(artifactOwner)
.put("/registry/v2/groups/{groupId}/artifacts/{artifactId}/owner")
.then()
.statusCode(400);
}

@Test
public void testMultipleGroups() throws Exception {
Expand Down Expand Up @@ -601,6 +639,93 @@ public void testUpdateArtifact() throws Exception {

}

@Test
public void testUpdateArtifactState() throws Exception {
String oaiArtifactContent = resourceToString("openapi-empty.json");
createArtifact("testUpdateArtifactState", "testUpdateArtifactState/EmptyAPI/1",ArtifactType.OPENAPI, oaiArtifactContent);

UpdateState updateState = new UpdateState();
updateState.setState(ArtifactState.DEPRECATED);

// Update the artifact state to DEPRECATED.
given()
.when()
.contentType(CT_JSON)
.pathParam("groupId", "testUpdateArtifactState")
.pathParam("artifactId", "testUpdateArtifactState/EmptyAPI/1")
.body(updateState)
.put("/registry/v2/groups/{groupId}/artifacts/{artifactId}/state")
.then()
.statusCode(204);

// Update the artifact state to DEPRECATED again.
given()
.when()
.contentType(CT_JSON)
.pathParam("groupId", "testUpdateArtifactState")
.pathParam("artifactId", "testUpdateArtifactState/EmptyAPI/1")
.body(updateState)
.put("/registry/v2/groups/{groupId}/artifacts/{artifactId}/state")
.then()
.statusCode(204);

// Send a GET request to check if the artifact state is DEPRECATED.
given()
.when()
.contentType(CT_JSON)
.pathParam("groupId", "testUpdateArtifactState")
.pathParam("artifactId", "testUpdateArtifactState/EmptyAPI/1")
.get("/registry/v2/groups/{groupId}/artifacts/{artifactId}")
.then()
.statusCode(200)
.header("X-Registry-Deprecated", "true");
}

@Test
public void testUpdateArtifactVersionState() throws Exception {
String oaiArtifactContent = resourceToString("openapi-empty.json");
createArtifact("testUpdateArtifactVersionState", "testUpdateArtifactVersionState/EmptyAPI",ArtifactType.OPENAPI, oaiArtifactContent);

UpdateState updateState = new UpdateState();
updateState.setState(ArtifactState.DEPRECATED);

// Update the artifact state to DEPRECATED.
given()
.when()
.contentType(CT_JSON)
.pathParam("groupId", "testUpdateArtifactVersionState")
.pathParam("artifactId", "testUpdateArtifactVersionState/EmptyAPI")
.pathParam("versionId", "1")
.body(updateState)
.put("/registry/v2/groups/{groupId}/artifacts/{artifactId}/versions/{versionId}/state")
.then()
.statusCode(204);

// Update the artifact state to DEPRECATED again.
given()
.when()
.contentType(CT_JSON)
.pathParam("groupId", "testUpdateArtifactVersionState")
.pathParam("artifactId", "testUpdateArtifactVersionState/EmptyAPI")
.pathParam("versionId", "1")
.body(updateState)
.put("/registry/v2/groups/{groupId}/artifacts/{artifactId}/versions/{versionId}/state")
.then()
.statusCode(204);

// Send a GET request to check if the artifact state is DEPRECATED.
given()
.when()
.contentType(CT_JSON)
.pathParam("groupId", "testUpdateArtifactVersionState")
.pathParam("artifactId", "testUpdateArtifactVersionState/EmptyAPI")
.pathParam("versionId", "1")
.get("/registry/v2/groups/{groupId}/artifacts/{artifactId}/versions/{versionId}")
.then()
.statusCode(200)
.header("X-Registry-Deprecated", "true");
}

@Test
@DisabledIfEnvironmentVariable(named = CURRENT_ENV, matches = CURRENT_ENV_MAS_REGEX)
@DisabledOnOs(OS.WINDOWS)
Expand Down
51 changes: 51 additions & 0 deletions app/src/test/java/io/apicurio/registry/rbac/AdminResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,47 @@ public void testGlobalRulesEndpoint() {
.statusCode(200)
.body(anything());
}

@Test
public void testCreateGlobalRule() throws Exception
{
//Test Rule type null
Rule nullType = new Rule();
nullType.setType(null);
nullType.setConfig("TestConfig");
given()
.when()
.contentType(CT_JSON)
.body(nullType)
.post("/registry/v2/admin/rules")
.then()
.statusCode(400);

//Test Rule config null
Rule nullConfig = new Rule();
nullConfig.setType(RuleType.VALIDITY);
nullConfig.setConfig(null);
given()
.when()
.contentType(CT_JSON)
.body(nullConfig)
.post("/registry/v2/admin/rules")
.then()
.statusCode(400);

//Test Rule config empty
Rule emptyConfig = new Rule();
emptyConfig.setType(RuleType.VALIDITY);
emptyConfig.setConfig("");
given()
.when()
.contentType(CT_JSON)
.body(emptyConfig)
.post("/registry/v2/admin/rules")
.then()
.statusCode(400);

}

@Test
public void testGlobalRules() throws Exception {
Expand Down Expand Up @@ -888,6 +929,16 @@ public void testRoleMappings() throws Exception {
.body("error_code", equalTo(404))
.body("message", equalTo("Role mapping not found for principal."));

//Update a mapping with null RoleType
update.setRole(null);
given()
.when()
.contentType(CT_JSON)
.body(update)
.put("/registry/v2/admin/roleMappings/TestUser")
.then()
.statusCode(400);

// Delete a role mapping
given()
.when()
Expand Down
Loading

0 comments on commit cd4b580

Please sign in to comment.