Add Discovery API for listing Features - #797
Conversation
| ListEntitiesResponse response = specService.listEntities(request.getFilter()); | ||
| responseObserver.onNext(response); | ||
| responseObserver.onCompleted(); | ||
| } catch (RetrievalException | IllegalArgumentException | InvalidProtocolBufferException e) { |
There was a problem hiding this comment.
Is it possible for you to return different gRPC exceptions here instead of INTERNAL for everything? Also, we should probably also catch and return unknown exceptions for the time being, because it can be hard to debug otherwise.
| public boolean hasAllEntities(List<String> entitiesFilter) { | ||
| List<String> allEntitiesName = | ||
| this.entities.stream() | ||
| .map(entitySpec -> entitySpec.toProto().getName()) |
There was a problem hiding this comment.
Why are you converting it to a proto?
| this.entities.stream() | ||
| .map(entitySpec -> entitySpec.toProto().getName()) | ||
| .collect(Collectors.toList()); | ||
| for (String entity : entitiesFilter) { |
There was a problem hiding this comment.
I think it would be easier to sort and compare the lists for equality.
| .collect(Collectors.toList()); | ||
| for (Feature feature : validFeatures) { | ||
| validFeaturesMap.put( | ||
| this.getProject().getName() + "/" + this.getName() + ":" + feature.getName(), feature); |
There was a problem hiding this comment.
Isn't this feature reference building available elsewhere in the code base?
| * @param labelsFilter contain labels that should be attached to FeatureSet's features | ||
| * @return Map of Feature references and Features | ||
| */ | ||
| public Map<String, Feature> getFeaturesByAllLabels(Map<String, String> labelsFilter) { |
There was a problem hiding this comment.
It seems strange that filtering is a part of this method. Can that be done externally with a static method? I would rather this be getFeaturesByRef and then have a filtering step outside.
| * filter | ||
| */ | ||
| public ListFeaturesResponse listFeatures(ListFeaturesRequest.Filter filter) | ||
| throws InvalidProtocolBufferException { |
There was a problem hiding this comment.
Where is InvalidProtocolBufferException being thrown and why do you leave it unhandled? It seems like we can add more context there.
| // Matching a specific project | ||
| featureSets = | ||
| featureSetRepository.findAllByNameLikeAndProject_NameOrderByNameAsc("%", project); | ||
| } else { |
There was a problem hiding this comment.
In what scenario would this else be reached?
| } | ||
|
|
||
| ListFeaturesResponse.Builder response = ListFeaturesResponse.newBuilder(); | ||
| if (featureSets.size() > 0) { |
There was a problem hiding this comment.
I don't think we need this conditional for size (I know it was in the other method)
| return FeatureSet.from_proto(get_feature_set_response.feature_set) | ||
|
|
||
| def list_entities(self) -> Dict[str, Entity]: | ||
| def list_features( |
There was a problem hiding this comment.
Can you please add minimal examples to these two methods?
https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html
| ) -> Dict[FeatureRef, Feature]: | ||
| """ | ||
| Returns a dictionary of entities across all feature sets | ||
| Returns a list of features based on filters provided |
There was a problem hiding this comment.
You are missing arguments in your docstring.
|
|
||
| return features_dict | ||
|
|
||
| def list_entities(self, project: str = None) -> Dict[str, Entity]: |
| "mocked_client", | ||
| [pytest.lazy_fixture("mock_client"), pytest.lazy_fixture("secure_mock_client")], | ||
| ) | ||
| def test_list_entities(self, mocked_client, mocker): |
There was a problem hiding this comment.
What part of the code are you testing here? Seems like you are testing almost no code.
| ("driver_id", driver_entity) | ||
| ]) | ||
|
|
||
| filter_by_project_entity_labels_expected = OrderedDict([ |
There was a problem hiding this comment.
I dont see a good reason for using OrderedDicts here.
woop
left a comment
There was a problem hiding this comment.
Not too far off the mark, but needs some changes. I would also recommend that you update the relevant documentation for these methods in our documentation.
c85cae3 to
d5e2fc7
Compare
2d33fe9 to
4fbd94c
Compare
|
/test test-end-to-end-batch |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: terryyylim, woop The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
|
/test test-end-to-end-batch |
|
/lgtm |
What this PR does / why we need it:
Adds listing functionality for Features.
Features
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: