yuqi1129 commented on PR #13846:
URL: https://github.com/apache/gravitino/pull/13846#issuecomment-6187884547
Thanks for the update in d4ac59ea2. The new `UnsupportedOperationException`
handling addresses the unsupported-view authorization error. I checked that
delta against the version I reviewed (`d0eff76cc9`), and two concerns remain:
1. **[Blocker] An absence probe must not authorize the subsequent load.**
In `LoadViewAuthorizationExecutor.execute`, the primary load permission
can be denied while an allowed probe observes that the view does not exist and
returns `true`. The interceptor then calls the REST handler. If another request
creates the same-named view between the probe and `ViewOperations.loadView`,
the handler loads and returns that view without another load-permission check.
A deterministic local probe using the real executor, interceptor, and
REST method, with mocked reads fixing this interleaving, returned HTTP 200
including the view SQL despite the primary load permission being false. This is
not a live Hive concurrency test, but it exercises the authorization-to-handler
boundary.
Please distinguish a successful absence probe from permission to load:
return a terminal, standard 404 response for the probe-only absent case,
without proceeding to the normal load handler. Simply throwing
`NoSuchViewException` from the executor would also need appropriate interceptor
handling so it does not become a generic authorization 500. Add a regression
test for creation between the absence observation and handler execution. The
table executor has the analogous pattern and should be followed up as well.
2. **[Important] The internal dispatcher’s `viewExists` is not a read-only
existence probe.**
The actual chain is `ViewNormalizeDispatcher.viewExists` → the default
`ViewCatalog.viewExists` → `ViewOperationDispatcher.loadView`. For an external
view not yet registered in Gravitino, this reaches schema loading and
`importView`, including `store.put`. Thus a caller with only probe permission
can trigger metadata import and write locking even though the request
ultimately returns 403. A local test using the real Core dispatcher confirmed
that `viewExists` imports the external view into the entity store.
Please provide a non-importing existence path that delegates to the
connector’s `viewExists`, preserving its existence semantics, rather than
routing through the dispatcher’s importing `loadView`. Add a test asserting
that a denied load with allowed probe permission does not import the view or
its parent schema.
Validation on `d0eff76cc9`: all 666 non-Docker server tests passed; the
temporary probes reproduced the race and import behavior. Reversing the
existence predicate made the new missing-view test fail as expected; after
restoration, the 20 relevant server tests passed. I have inspected the new
commit’s delta, but have not rerun the full suite on `d4ac59ea2` or run live
Flink/Hive integration tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
