From 041083a45093a4cf2641c883ae49bd4ddb5ccce1 Mon Sep 17 00:00:00 2001 From: e10112844 Date: Mon, 3 Aug 2020 13:00:10 -0400 Subject: [PATCH 1/2] fix: apply feature set should authorize default project if project is not set. --- .../java/feast/core/grpc/CoreServiceImpl.java | 6 +++-- .../java/feast/core/service/SpecService.java | 24 ++++++++++++------- .../feast/core/auth/CoreServiceAuthTest.java | 3 +++ .../feast/core/service/SpecServiceTest.java | 12 ++++------ 4 files changed, 26 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/feast/core/grpc/CoreServiceImpl.java b/core/src/main/java/feast/core/grpc/CoreServiceImpl.java index 19d73f8f8f..b2545d2345 100644 --- a/core/src/main/java/feast/core/grpc/CoreServiceImpl.java +++ b/core/src/main/java/feast/core/grpc/CoreServiceImpl.java @@ -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; @@ -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) { diff --git a/core/src/main/java/feast/core/service/SpecService.java b/core/src/main/java/feast/core/service/SpecService.java index 4365b8fd6c..d90f7b7013 100644 --- a/core/src/main/java/feast/core/service/SpecService.java +++ b/core/src/main/java/feast/core/service/SpecService.java @@ -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); @@ -383,6 +374,21 @@ public ApplyFeatureSetResponse applyFeatureSet(FeatureSetProto.FeatureSet newFea .build(); } + /** + * sets project to 'default' if project is not specified in featureSet + * + * @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. * diff --git a/core/src/test/java/feast/core/auth/CoreServiceAuthTest.java b/core/src/test/java/feast/core/auth/CoreServiceAuthTest.java index f9e8becd0e..84069598b3 100644 --- a/core/src/test/java/feast/core/auth/CoreServiceAuthTest.java +++ b/core/src/test/java/feast/core/auth/CoreServiceAuthTest.java @@ -102,6 +102,9 @@ public void shouldNotApplyFeatureSetIfNotProjectMember() throws InvalidProtocolB StreamRecorder 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 = diff --git a/core/src/test/java/feast/core/service/SpecServiceTest.java b/core/src/test/java/feast/core/service/SpecServiceTest.java index 48572ddcec..03b16543ba 100644 --- a/core/src/test/java/feast/core/service/SpecServiceTest.java +++ b/core/src/test/java/feast/core/service/SpecServiceTest.java @@ -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); @@ -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 From 841f9e7b7ce49dc886f60ac373dd228204fab873 Mon Sep 17 00:00:00 2001 From: Willem Pienaar <6728866+woop@users.noreply.github.com> Date: Wed, 5 Aug 2020 11:42:27 +0800 Subject: [PATCH 2/2] Update comment --- core/src/main/java/feast/core/service/SpecService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/feast/core/service/SpecService.java b/core/src/main/java/feast/core/service/SpecService.java index d90f7b7013..1523e847bd 100644 --- a/core/src/main/java/feast/core/service/SpecService.java +++ b/core/src/main/java/feast/core/service/SpecService.java @@ -375,7 +375,7 @@ public ApplyFeatureSetResponse applyFeatureSet(FeatureSetProto.FeatureSet newFea } /** - * sets project to 'default' if project is not specified in featureSet + * Sets project to 'default' if project is not specified in feature set * * @param featureSet Feature set which needs to be imputed with default project. */