Enhance spec-filter to support x-operation-group filtering and merge parameters across operations in same x-operation-group instead of selecting by max parameters#374
Conversation
…parameters across operations in same x-operation-group instead of selecting by max parameters Signed-off-by: xil <fridalu66@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #374 +/- ##
===========================================
+ Coverage 43.89% 56.19% +12.30%
===========================================
Files 17 17
Lines 1383 1397 +14
Branches 371 378 +7
===========================================
+ Hits 607 785 +178
+ Misses 672 528 -144
+ Partials 104 84 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lucy66hw
requested review from
amberzsy,
karenyrx and
philiplhchan
as code owners
| paths: | ||
| - /{index}/_bulk | ||
| - /{index}/_search | ||
| /{index}/_bulk: |
There was a problem hiding this comment.
how do we join the two /_bulk and /{index}/_bulk together, since both share the same x-operation-group: bulk, but the /_bulk endpoint has one less parameter?
- $ref: '#/components/parameters/bulk::path.index'
There was a problem hiding this comment.
we don't join both. only join specified path with same group.
There was a problem hiding this comment.
so users can technically add both of these to spec-filter.yaml right:
/_bulk:
x-operation-group:
- bulk
/{index}/_bulk
x-operation-group:
- bulk
but both endpoints will be named after the x-operation-group, so both RPC endpoints will be named Bulk(). How do we prevent/consolidate that?
There was a problem hiding this comment.
To me, it feels like x-opertion-group should be the top-level one then. Since x-operation-group ≈ endpoint and the REST API endpoint (e.g. /_bulk or /{index}/_bulk) is what determines the schema (the Protobuf message content)
There was a problem hiding this comment.
we only keep the x-operation-group. Any endpoint with the included x-operation-group will be joined together and use merged parameter.
| - /{index}/_search | ||
| /{index}/_bulk: | ||
| x-operation-group: | ||
| - bulk |
There was a problem hiding this comment.
why does the operation group need to be list instead of a singular string field? are there examples in the spec where endpoints correspond to more than 1 x-operation-group?
There was a problem hiding this comment.
for API: /{index}/_doc/{id}, it has get, exist, index and delete operation.
| } from '../../src/utils/helper'; | ||
| import { OpenAPIV3 } from 'openapi-types'; | ||
|
|
||
| describe('getSchemaNames', () => { |
There was a problem hiding this comment.
nice, thanks for the test coverage
|
Also noticed that the PR does not actually |
The changes are in tools/proto-convert/src/config/protobuf-schema-template/partial_header.mustache file. |
Signed-off-by: xil <fridalu66@gmail.com>
Signed-off-by: Xi Lu <fridalu66@gmail.com>
2 participants
Description
Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.