[ATTRIBUTES][ENDPOINT] Row-Based filtering for attributes#464
[ATTRIBUTES][ENDPOINT] Row-Based filtering for attributes#464ChanukaUOJ wants to merge 7 commits intoLDFLK:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the OpenGIN Read API by introducing row-based filtering capabilities for attribute data. This allows clients to retrieve highly specific subsets of attribute information by applying detailed filter conditions directly to the data rows, improving data access precision and efficiency. The change involved updating the API contract, modifying the core service to handle and apply these filters, and expanding the test suite to ensure robustness. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces row-level filtering for tabular attributes in the ReadEntity API. The /entities/{entityId}/attributes/{attributeName} endpoint in read_api.yaml was changed from GET to POST and now accepts a request body for records to specify row filters. The core-api was updated to extract these record filters from attributes and pass them to the GetData repository method, which now includes logic to construct SQL WHERE clauses based on these filters and their operators (e.g., eq, neq, gt, lt, gte, lte, contains, notcontains). Corresponding test cases were added in data_handler_test.go to validate the new row filtering functionality. The read-api service was updated to handle the new POST request and integrate the row filters into the ReadEntityRequest. Review comments highlight the need for input validation in extractRowsFromTabularAttributes to prevent invalid SQL queries, a bug in a test assertion in data_handler_test.go, and several instances of logging potentially sensitive user-supplied data (record filters, general filters, SQL arguments) which should be removed or masked. Additionally, there's a suggestion to refactor duplicated test setup code in read_api_service_test.bal for better maintainability and to reduce verbose debug logging in data_handler.go.
|
@ChanukaUOJ first comment is this change is too big to review. Is it possible to do the following. Is this possible? If possible, I would rather go with that approach. |
|
|
| items: | ||
| type: string | ||
| default: ["*"] | ||
| requestBody: |
There was a problem hiding this comment.
Put a description for the request body. You can explain how to use it for filtering and what are the accepted filter operators. This will show in the swagger documentation
| var filters []postgres.RecordFilter | ||
| for _, val := range listValue.Values { | ||
| datum := val.GetStructValue() | ||
| if datum == nil { |
There was a problem hiding this comment.
how would we get a nil datum here?
There was a problem hiding this comment.
val.GetStructValue() return nil if the val is not an object. This is for nil safety
| FieldName string | ||
| Operator string | ||
| Value string |
There was a problem hiding this comment.
Maybe define the Operator specifically? (What can you pass here, etc?)
| } | ||
|
|
||
| // Default: simple equality filter | ||
| whereClauses = append(whereClauses, fmt.Sprintf("%s = $%d", commons.SanitizeIdentifier(key), argCount)) |
There was a problem hiding this comment.
keeping this would be redundant?
There was a problem hiding this comment.
If any case key != "record_filters", it perform this as a fallback.
vibhatha
left a comment
vibhatha
left a comment
There was a problem hiding this comment.
@ChanukaUOJ I added some comments regarding the core API changes. Let's fix those and move forward to the other Read API.
| assert.Len(t, filteredRows, 1) | ||
| assert.Equal(t, expectedRows[0], filteredRows[0].([]interface{})) | ||
| } | ||
|
|
There was a problem hiding this comment.
Test for trying to compare something that is not a string? What would happen?
There was a problem hiding this comment.
Since Value in recordFilter struct is defined as string. always it should be a string.
| } | ||
|
|
||
| // extractRowsFromTabularAttributes extracts row filters from a tabular attribute value | ||
| func extractRowsFromTabularAttributes(anyValue *anypb.Any) ([]postgres.RecordFilter, error) { |
There was a problem hiding this comment.
I am not sure about this name. This function is extracting the filters passed by the user, it's not technically the "rows". Even though from the read api the param is named rows
There was a problem hiding this comment.
changed into extractRecordFiltersFromTabularAttributes
4 participants
Description
This PR introduces a new functionality that enables row-based filtering in the OpenGIN Read API service. The implementation allows clients to apply filtering conditions when retrieving attribute data.
The targeted API endpoint for this feature is:
/entities/{entityId}/attributes/{attributeName}This endpoint has been updated according to the Read API contract to support the new filtering capability.
Changes
GETtoPOSTto support filter payloads.read_api_service.balto implement the new endpoint behavior.types.balwith the required type definitions for filtering.read_api_service_test.bal.serviceanddata_handlerimplementations in the core service.data_handler_test.go.Testing
Additional Information
This PR closes #461.