Conversation
❌ 4 blocking issues (6 total)
|
| end | ||
|
|
||
| def search(searchterm:, citation:, contributors:, funding_information:, geodistance:, geobox:, identifiers:, | ||
| locations:, subjects:, title:, index:, source:, from:, boolean_type:, fulltext:, per_page: 20, |
Why these changes are being introduced: Semantic tuning arguments are accepted at runtime, but are intentionally hidden from GraphQL introspection by custom visibility behavior in the schema. This creates a mismatch for downstream clients that validate against introspection-derived schema artifacts, including graphql-client. It was decided in [ADR 8](https://github.com/MITLibraries/timdex/blob/main/docs/architecture-decisions/0008-expose-tuning-parameters-in-public-graphql-schema.md) to expose these arguments. Relevant ticket(s): - [USE-680](https://mitlibraries.atlassian.net/browse/USE-680) How this addresses that need: This adds a Tuning Parameters Input Type that exposes the semantic tuning arguments and validates acceptable input ranges, as specified in the ADR. Side effects of this change: We no longer use the `GraphQL::Schema::Visibility` module. This should be fine, as it was only needed to call the `visible?` method that hid internal arguments.
There was a problem hiding this comment.
Pull request overview
This PR updates TIMDEX’s public GraphQL schema to explicitly expose semantic-search tuning parameters (per ADR 8), removing the prior approach of hiding “internal” arguments from introspection so downstream clients can rely on introspection-derived schema artifacts.
Changes:
- Replaces the previously hidden semantic tuning arguments with a new
tuningParametersInputinput object argument onsearch. - Adds runtime validation for tuning parameter ranges and returns GraphQL execution errors for invalid values.
- Updates/extends controller tests to assert introspection visibility and validate accept/reject behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/controllers/graphql_controller_test.rb | Updates introspection expectations and adds tests for valid/invalid tuning parameter inputs. |
| app/graphql/types/tuning_parameters_input_type.rb | Introduces a new GraphQL input object to expose tuning parameters in the public schema. |
| app/graphql/types/query_type.rb | Adds tuningParametersInput to search and validates/constructs semantic_options from it. |
| app/graphql/timdex_schema.rb | Removes schema visibility customization now that internal-only arguments are no longer used/hidden. |
Suppressed comments (2)
app/graphql/types/query_type.rb:225
- This validation error message doesn’t indicate that the field is nested under
tuningParametersInput, which can be confusing for clients. Consider prefixing the field name with the input object path.
unless threshold.between?(0.0, 1.0)
raise GraphQL::ExecutionError, "dropBoostThreshold must be between 0.0 and 1.0, got #{threshold}"
end
app/graphql/types/query_type.rb:235
- This validation error message doesn’t indicate that the field is nested under
tuningParametersInput, which can be confusing for clients. Consider prefixing the field name with the input object path.
unless tokens.positive?
raise GraphQL::ExecutionError,
"shortQueryMaxTokens must be greater than 0, got #{tokens}"
end
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Remove trailing whitespace from argument description Make error messages more specific
| semantic_options[:short_query_max_tokens] = tokens | ||
| end | ||
|
|
||
| semantic_options |
There was a problem hiding this comment.
Found 3 issues:
1. Function with high complexity (count = 10): validate_and_build_semantic_options [qlty:function-complexity]
2. Assignment Branch Condition size for validate_and_build_semantic_options is too high. [<7, 19, 7> 21.42/17] [rubocop:Metrics/AbcSize]
3. Cyclomatic complexity for validate_and_build_semantic_options is too high. [8/7] [rubocop:Metrics/CyclomaticComplexity]
Why these changes are being introduced:
Semantic tuning arguments are accepted at runtime, but are intentionally hidden from GraphQL
introspection by custom visibility behavior in the schema. This creates a mismatch for downstream
clients that validate against
introspection-derived schema artifacts, including
graphql-client.
It was decided in ADR 8 to expose these arguments.
Relevant ticket(s):
How this addresses that need:
This adds a Tuning Parameters Input Type that
exposes the semantic tuning arguments and validates acceptable input ranges, as specified in the ADR.
Side effects of this change:
We no longer use the
GraphQL::Schema::Visibilitymodule. This should be fine, as it was only needed to call thevisible?method that hid internalarguments.
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO