Skip to content

Commit

Permalink
Impute default project if empty before authorization is called (#926)
Browse files Browse the repository at this point in the history
* fix: apply feature set should authorize default project if project is not set.

* Update comment

Co-authored-by: Willem Pienaar <6728866+woop@users.noreply.github.com>
  • Loading branch information
jmelinav and woop authored Aug 7, 2020
1 parent d2d47e1 commit f537863
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 19 deletions.
6 changes: 4 additions & 2 deletions core/src/main/java/feast/core/grpc/CoreServiceImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import feast.core.service.StatsService;
import feast.proto.core.CoreServiceGrpc.CoreServiceImplBase;
import feast.proto.core.CoreServiceProto.*;
import feast.proto.core.FeatureSetProto.FeatureSet;
import io.grpc.Status;
import io.grpc.StatusRuntimeException;
import io.grpc.stub.StreamObserver;
Expand Down Expand Up @@ -187,9 +188,10 @@ public void applyFeatureSet(
String projectId = null;

try {
projectId = request.getFeatureSet().getSpec().getProject();
FeatureSet featureSet = specService.imputeProjectName(request.getFeatureSet());
projectId = featureSet.getSpec().getProject();
authorizationService.authorizeRequest(SecurityContextHolder.getContext(), projectId);
ApplyFeatureSetResponse response = specService.applyFeatureSet(request.getFeatureSet());
ApplyFeatureSetResponse response = specService.applyFeatureSet(featureSet);
responseObserver.onNext(response);
responseObserver.onCompleted();
} catch (org.hibernate.exception.ConstraintViolationException e) {
Expand Down
24 changes: 15 additions & 9 deletions core/src/main/java/feast/core/service/SpecService.java
Original file line number Diff line number Diff line change
Expand Up @@ -312,15 +312,6 @@ public ListStoresResponse listStores(ListStoresRequest.Filter filter) {
@Transactional
public ApplyFeatureSetResponse applyFeatureSet(FeatureSetProto.FeatureSet newFeatureSet)
throws InvalidProtocolBufferException {
// Autofill default project if not specified
if (newFeatureSet.getSpec().getProject().isEmpty()) {
newFeatureSet =
newFeatureSet
.toBuilder()
.setSpec(newFeatureSet.getSpec().toBuilder().setProject(Project.DEFAULT_NAME).build())
.build();
}

// Validate incoming feature set
FeatureSetValidator.validateSpec(newFeatureSet);

Expand Down Expand Up @@ -383,6 +374,21 @@ public ApplyFeatureSetResponse applyFeatureSet(FeatureSetProto.FeatureSet newFea
.build();
}

/**
* Sets project to 'default' if project is not specified in feature set
*
* @param featureSet Feature set which needs to be imputed with default project.
*/
public FeatureSetProto.FeatureSet imputeProjectName(FeatureSetProto.FeatureSet featureSet) {
if (featureSet.getSpec().getProject().isEmpty()) {
return featureSet
.toBuilder()
.setSpec(featureSet.getSpec().toBuilder().setProject(Project.DEFAULT_NAME).build())
.build();
}
return featureSet;
}

/**
* UpdateStore updates the repository with the new given store.
*
Expand Down
3 changes: 3 additions & 0 deletions core/src/test/java/feast/core/auth/CoreServiceAuthTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ public void shouldNotApplyFeatureSetIfNotProjectMember() throws InvalidProtocolB

StreamRecorder<ApplyFeatureSetResponse> responseObserver = StreamRecorder.create();
FeatureSetProto.FeatureSet incomingFeatureSet = newDummyFeatureSet("f2", 1, project).toProto();
doReturn(incomingFeatureSet)
.when(specService)
.imputeProjectName(any(FeatureSetProto.FeatureSet.class));
FeatureSetProto.FeatureSetSpec incomingFeatureSetSpec =
incomingFeatureSet.getSpec().toBuilder().build();
FeatureSetProto.FeatureSet spec =
Expand Down
12 changes: 4 additions & 8 deletions core/src/test/java/feast/core/service/SpecServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ public void applyFeatureSetShouldCreateProjectWhenNotAlreadyExists()
}

@Test
public void applyFeatureSetShouldUsedDefaultProjectIfUnspecified()
public void imputeProjectNameShouldSetDefaultProjectIfUnspecified()
throws InvalidProtocolBufferException {
Feature f3f1 = TestUtil.CreateFeature("f3f1", Enum.INT64);
Feature f3f2 = TestUtil.CreateFeature("f3f2", Enum.INT64);
Expand All @@ -616,13 +616,9 @@ public void applyFeatureSetShouldUsedDefaultProjectIfUnspecified()
FeatureSetProto.FeatureSet incomingFeatureSet =
TestUtil.CreateFeatureSet("f3", "", Arrays.asList(f3e1), Arrays.asList(f3f2, f3f1))
.toProto();
ApplyFeatureSetResponse applyFeatureSetResponse =
specService.applyFeatureSet(incomingFeatureSet);
assertThat(applyFeatureSetResponse.getStatus(), equalTo(Status.CREATED));

assertThat(
applyFeatureSetResponse.getFeatureSet().getSpec().getProject(),
equalTo(Project.DEFAULT_NAME));
FeatureSetProto.FeatureSet imputedFeatureSet =
specService.imputeProjectName(incomingFeatureSet);
assertThat(imputedFeatureSet.getSpec().getProject(), equalTo(Project.DEFAULT_NAME));
}

@Test
Expand Down

0 comments on commit f537863

Please sign in to comment.