LRriver commented on PR #351: URL: https://github.com/apache/hugegraph-ai/pull/351#issuecomment-4621281369
> @LRriver > > thanks for the detailed review. here's what i'm taking and where i landed on the open calls. > > api contract: > > * remove triples, property-graph only for this PR. it never actually worked here: post_deal only returns vertices/edges, and InfoExtract only emits triples when no schema is set, but this flow always sets one. can add it later with a real triples shape. > * add GraphExtractResponse (status / result / warnings / meta) instead of returning the raw flow dict. > * add a dedicated GraphExtractClientConfig (all-optional, extra="forbid") so the implicit 127.0.0.1:8080 / hugegraph defaults can't kick in. > * drop the request-level url override and use the configured server url. can add a server-side allowlist later if multi-server support is needed. > * keeping the strict request-local checks already added (named schema requires local config, client_config.graph must match the schema name, no field-by-field fallback into globals). > > tests: > > * response envelope shape (status / result / warnings / meta) > * texts accepting both a string and a list > * mismatched client_config.graph rejected > * inline schema + client_config behavior > * property-graph-only output > * /rag, /text2gremlin, /config/graph still registering > > structural: > > * moving the models to graph_extract_requests.py / graph_extract_responses.py and the route to graph_extract_api.py, both in this PR. > * the GraphExtractService.extract_sync(req) wrapper is the one i'd like your call on: happy to add it here, or land it as a quick follow-up so this PR stays focused on the contract + security fixes. which do you prefer? I agree with the proposed scope choices: - Keep this PR property-graph only and leave `triples` for a later PR with a real request/response contract. - Add the `GraphExtractResponse` envelope instead of returning the raw flow dict. - Use dedicated graph extract request/response/config models instead of reusing RAG or global graph config models. - Drop request-level URL override for this PR. - Move the route and models into graph-extract-specific modules. The one point I would adjust is the service boundary: please include the thin `GraphExtractService.extract_sync(req)` wrapper in this PR. The wrapper should only move the existing synchronous extraction path out of the FastAPI handler: scheduler invocation, raw result parsing, response normalization, and error mapping. It should not add async jobs, graph import, extract-and-import, or a broader service abstraction in this PR. This keeps the public route small and makes the API behavior easier to test before merge. A few details should also be covered before merging: - Since request-level URL override is being dropped, please make sure `url` is not accepted in `GraphExtractClientConfig`; with `extra="forbid"`, a request containing `client_config.url` should fail validation instead of being silently ignored. - Inline schema validation should validate the actual schema shape, not only the presence of `vertexlabels` and `edgelabels`. Reusing `CheckSchema(deepcopy(schema)).run()` or the existing schema validation path in the request validator would keep invalid schema input as a 422 before the scheduler runs. - `WkFlowInput.reset()` should clear `graph_client_config` to avoid leaking request-scoped connection settings across reused flow inputs. With those changes, the PR can stay focused while still landing a clean public API boundary. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
