Add HTTP integration GraphQL query
What does this MR do?
Related to #321674 (closed)
Adds a new alertManagementHttpIntegration
to fetch information about a single HTTP Integration by ID.
alertManagementIntegration
query similar to alertManagementHttpIntegration
?
Why there is no Alert Management Integration consists of two kinds: HTTP Integrations and Prometheus integration. The last can only be present in a single copy and I think there is no need to filter it by ID when it's already fetched from the alertManagementIntegrations
query.
If we ignore filtering Prometheus integration in alertManagementIntegration
, then alertManagementIntegration
and alertManagementHttpIntegration
will do exactly the same job.
One may argue, in the future, we may have more integration types and alertManagementIntegration
is going to allow us to filter any of them by ID. I would suggest getting back to that discussion once we have more integration types.
GraphQL
Query
query Extract($fullPath: ID!, $id: AlertManagementHttpIntegrationID!) {
project(fullPath: $fullPath) {
alertManagementHttpIntegration(id: $id) {
id
active
name
payloadExample
payloadAttributeMappings {
fieldName
label
type
}
}
}
}
Query variables
{
"fullPath": "<PROJECT-FULL-PATH>",
"id": "gid://gitlab/AlertManagement::HttpIntegration/<ID>"
}
Screenshots (strongly suggested)
Does this MR meet the acceptance criteria?
Conformity
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides - [-] Database guides
-
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. - [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
- [-] Label as security and @ mention
@gitlab-com/gl-security/appsec
- [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [-] Security reports checked/validated by a reviewer from the AppSec team
Merge request reports
Activity
changed milestone to %13.10
assigned to @ck3g
added documentation label
1 Warning ⚠ This MR has a Changelog file outside ee/
, but code changes inee/
. Consider moving the Changelog file intoee/
.1 Message 📖 This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
doc/api/graphql/reference/index.md
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
Category Reviewer Maintainer backend Robert May ( @robotmay_gitlab
) (UTC+0, 1 hour behind@ck3g
)Dmitriy 'DZ' Zaporozhets ( @dzaporozhets
) (UTC+2, 1 hour ahead of@ck3g
)~changelog No reviewer available No maintainer available If needed, you can retry the
danger-review
job that generated this comment.Generated by
🚫 DangerEdited by 🤖 GitLab Bot 🤖added 2 commits
mentioned in merge request !54702 (closed)
@robotmay_gitlab could you please review? Thank
@robotmay_gitlab pinging. Maybe the previous notification got lost
Sorry for the delay @ck3g. I missed the previous message in a flurry of others
😅 This looks good to me, though I'm not an expert in GraphQL. I'll pass this on to @alexkalderimis as I know he's more experienced in it than me
😺
assigned to @robotmay_gitlab and unassigned @ck3g
requested review from @robotmay_gitlab
assigned to @alexkalderimis and unassigned @robotmay_gitlab
requested review from @alexkalderimis
mentioned in issue gitlab-com/www-gitlab-com#6605 (closed)
279 279 description: 'HTTP Integrations which can receive alerts for the project.', 280 280 resolver: Resolvers::AlertManagement::HttpIntegrationsResolver 281 281 282 field :alert_management_http_integration, We already have a field on the
ProjectType
that seems suitable for this:Project.alertManagementHttpIntegrations
(I notice we also haveProject.alertManagementIntegrations
- I am worried that we are accumulating several very similar fields onProject
that might be confusing to users.Did you consider other approaches here, such as adding filter arguments to
Project.alertManagementIntegrations
?I was considering two approaches here. The one with the separate
Project.alertManagementHttpIntegration
and another one is adding a new filtering argument to theProject.alertManagementHttpIntegrations
.I chose the new field approach due to its simplicity and single responsibility.
But I don't mind flipping the solution in favor of
Project.alertManagementHttpIntegrations
.I didn't add the field to
Project.alertManagementIntegrations
because (I was trying to explain that in the MR description) that field returns two kinds of integrations: HTTP Integrations and the Prometheus service.The Prometheus service in there is always a single record (or blank I guess). That, I think, makes filtering by ID is a little bit tricky, because we need to identify the type to which is that ID belongs. Then, if the ID belongs to Prometheus, it doesn't make sense to filter by its ID again, because it's always a single record. Here we can only filter by HTTP integrations, but I thought it's could be odd to dismiss one of the types.
Project.alertManagementHttpIntegrations
on the other hand is only focused on HTTP integration, which has a couple of specific pieces of information we need when we edit one of the integration's settings.I didn't add the field to
Project.alertManagementIntegrations
because (I was trying to explain that in the MR description) that field returns two kinds of integrations: HTTP Integrations and the Prometheus service.that is correct, but it doesn't seem like a large problem. We can distinguish which kind of integration this is with
id.model_class
.To filter by ID we need to get the ID. Get it
id.model_class
. Then check if the model class is::AlertManagement::HttpIntegration
.If it's an
HttpIntegration
we pass the integer ID to::AlertManagement::HttpIntegrationsFinder
to filter the HTTP Integrations. https://gitlab.com/gitlab-org/gitlab/-/blob/d3250ebe5506b66bb7d019841a1f66981456974b/app/graphql/resolvers/alert_management/integrations_resolver.rb#L25If it's not an
HttpIntegration
we compare theproject.prometheus_service
ID with the ID we have, if return the Prometheus service if IDs match.Did I understand that right?
2 3 module Resolvers 4 module AlertManagement 5 class HttpIntegrationResolver < BaseResolver 6 alias_method :project, :synchronized_object 7 8 type Types::AlertManagement::HttpIntegrationType, null: true 9 10 argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration], 11 required: true, 12 description: 'ID of the integration.' 13 14 def resolve(**args) 15 return unless Ability.allowed?(current_user, :admin_operations, project) 16 17 GitlabSchema.object_from_id(args[:id], expected_class: ::AlertManagement::HttpIntegration) Praise:
this will be inherently N+1 safe due to the way we load objects.
However:
- we already know the type: we do not need the
expected_class:
here. The typed ID makes that unnecessary - we do need to coerce the input until we complete #257883 (closed)
See https://gitlab.com/gitlab-org/gitlab/blob/cd33b34e16189028b009d724425a4d20f583f0f5/app/graphql/mutations/boards/update.rb#L38 for an example of this.
- this is a very very simple resolver - does it need to be a resolver? Could it just be a method?
- we already know the type: we do not need the
1 # frozen_string_literal: true 2 3 module Resolvers 4 module AlertManagement 5 class HttpIntegrationResolver < BaseResolver 6 alias_method :project, :synchronized_object 7 8 type Types::AlertManagement::HttpIntegrationType, null: true 9 10 argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration], 11 required: true, 12 description: 'ID of the integration.' 13 14 def resolve(**args) 15 return unless Ability.allowed?(current_user, :admin_operations, project) We have field authorizations working now (huzzah!)
That means this line (and the entire implementation) could be moved to the field instead, as:
# in project_type.rb field :alert_management_http_integration type: Types::AlertManagement::HttpIntegrationType, null: true, description: 'A single HTTP integration which can receive alerts for the project.', authorize: :admin_operations do argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration], required: true, description: 'ID of the integration.' end def alert_management_http_integration(id:) id = Types::AlertManagement::HttpIntegrationType.coerce_isolated_input(id) GitlabSchema.find_by_gid(id) end
we could even leverage
:loads
to make this even smaller, but I think this needs to wait for #257883 (closed) to be fully robust:# in project_type.rb field :alert_management_http_integration type: Types::AlertManagement::HttpIntegrationType, null: true, description: 'A single HTTP integration which can receive alerts for the project.', authorize: :admin_operations do argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration], required: true, loads: Types::AlertManagement::HttpIntegrationType, as: :integration, description: 'ID of the integration.' end def alert_management_http_integration(integration:) integration end
Do we really need a resolver?
12 let_it_be(:project) { create(:project) } 13 let_it_be(:integration) { create(:alert_management_http_integration, project: project) } 14 let_it_be(:args) { { id: global_id_of(integration) } } 15 16 subject { sync(resolve_http_integration(args: args)) } 17 18 before do 19 project.add_developer(developer) 20 project.add_maintainer(maintainer) 21 end 22 23 specify do 24 expect(described_class).to have_nullable_graphql_type(Types::AlertManagement::HttpIntegrationType) 25 end 26 27 context 'when user does not have permission' do Hi @ck3g
Thanks for this, but I think I need convincing that:
- we need a new field for this
- that the field needs to go under project
- that we need a resolver in the first place
Could you please explain these choices?
Also, I think we are definitely missing a few tests. I think we can skip the N+1 specs, since the implementation is so simple it is guaranteed to be correct.
Edited by Alex KalderimisThanks for the detailed review @alexkalderimis
I've added an explanation about the choice between the two solutions. But I'm fine switching to the solution you're suggesting. Let me know which one you think would be the best here.
Also, I think we are definitely missing a few tests.
What tests are we missing here? I'll add them
What tests are we missing here? I'll add them
I'm very sorry - I appear not to have posted a comment.
We need a test that a user with access to project
A
and projectB
cannot request integrationA.x
from projectB
.There is no security risk here, but there is the risk of really confusing data (and just incorrect modelling) if we allow loading the integrations of one project underneath another.
i.e.:
query($id: AlertManagementHttpIntegrationID!) { a: project(fullPath: "a/b") { alertManagementHttpIntegration(id: $id) { name } } b: project(fullPath: "b/a") { alertManagementHttpIntegration(id: $id) { name } } }
can logically return
{a: nil, b: nil}
,{a: nil, b: something}
and{a: something, b: nil}
but never:{a: something, b: something}
.To summarize, those are the changes I need to make:
- Move filtering by ID to
Project.alertManagementHttpIntegrations
- Also add filtering by ID tp
Project.alertManagementIntegrations
using a trick withid.model_class
(described here !56647 (comment 534605189)) - Cover the scenario (if not yet covered) where a user cannot access an Integration that doesn't belong to the current project (from this question !56647 (comment 534607715))
In this case, the question about resolvers is off the table, because we'll extend existing resolvers.
@alexkalderimis Is everything correct here? Did I miss something?
- Move filtering by ID to
Yes I think that makes sense.
However:
I am not sure what the front-end requirements are here, but we could avoid (3) by moving this field to
Query
and allow a single integration to be retrieved by ID (without the nesting, we would not need to verify consistency - and we already have a policy check on the class itself).So if we choose to model this as a nested field on
Project
, then we need to ensure that data is consistent. But we don't have to model it this way.There are tradeoffs to both positions in regard to (1) and (2). i.e. both adding a new field and adding an argument to an existing one add complexity. It really depends where we want that complexity to go, and how large we want the schema to get. What is the front-end requirement here? Are we prepared to maintain a large number of alert-management fields in the project type? Or would it be better to have a more complex resolver?
I really am not insisting here - I am happy to be persuaded depending on what is right for you and your team.
The main requirement for the frontend now is to be able to get a couple of extra fields which belong only to HTTP Integrations.
The integrations can be accessed only on the project level, so we don't need to get integrations for several projects at once. Also, there won't be many integrations per project.
At the moment, there is no need to filter non HTTP integrations, so implementing this filter is mostly for sync functionality between
alertManagementIntegrations
andalertManagementHttpIntegrations
. That's why I was trying to avoid that in the first place. I would still rather avoid doing that because we're going to implement a piece of functionality we don't need at the moment.The main requirement for the frontend now is to be able to get a couple of extra fields which belong only to HTTP Integrations.
we can do this now with inline fragments, no?
The integrations can be accessed only on the project level, so we don't need to get integrations for several projects at once.
this is a public API - there are no guarantees on how clients will use it. If we advertise an entity as nested beneath another, that is a claim we need to be able to assure clients of.
Also, there won't be many integrations per project.
is there a hard limit? I see right now we are returning arrays (i.e. we are loading all integrations in memory on each request). Do we have an issue to track abuse of this feature?
At the moment, there is no need to filter non HTTP integrations, so implementing this filter is mostly for sync functionality between
alertManagementIntegrations
andalertManagementHttpIntegrations
so we are implementing a new field, to sync data between two other fields that only differ in their return type, and the type of one is an interface implemented by the other? That seems really odd to me. Surely, with fragments to select the fields we need, we just don't need that.
Edited by Alex KalderimisThe integrations can be accessed only on the project level, so we don't need to get integrations for several projects at once.
this is a public API - there are no guarantees on how clients will use it. If we advertise an entity as nested beneath another, that is a claim we need to be able to assure clients of.
Here I was actually defending the need to limit the query to filter only integrations of a specified project.
is there a hard limit? I see right now we are returning arrays (i.e. we are loading all integrations in memory on each request). Do we have an issue to track abuse of this feature?
Not sure about that. Probably not
so we are implementing a new field, to sync data between two other fields that only differ in their return type, and the type of one is an interface implemented by the other? That seems really odd to me. Surely, with fragments to select the fields we need, we just don't need that.
I'm lost here, I'm sorry. Let me rephrase it.
I was trying to say, that there are two similar types
alertManagementIntegrations
andalertManagementHttpIntegrations
. The last one is the subset of the first. On top of thatHttpIntegrations
can return additional information, whichIntegrations
don't have. When we edit an HTTP integration we need to get that additional information. That's why we need to filter HTTP integrations by the ID.At the moment, we don't need to filter all integrations by ID. If we're going to add filtering to
alertManagementIntegrations
by ID, that's only if we want to have the same (or similar) filtering functionality in both these types.That's why I was trying to avoid adding new code, which we're not going to use at the moment.
I was trying to say, that there are two similar types
alertManagementIntegrations
andalertManagementHttpIntegrations
. The last one is the subset of the first. On top of thatHttpIntegrations
can return additional information, whichIntegrations
don't have.yes, thank you for clarifying. I saw that when I explored the code. I see that we have
app/graphql/types/alert_management/integration_type.rb
which defines an interface implemented byapp/graphql/types/alert_management/http_integration_type.rb
. This means that any field that returnsAlertManagementIntegration
entities may be returning objects of the concrete typeAlertManagementHttpIntegration
.The system in GraphQL for dealing with this (selecting fields of implementations of interfaces that do not belong on the interface) is called inline interfaces.
That would look like:
query { project(fullPath: "a/b") { alertManagementIntegrations { nodes { id type name ... on AlertManagementHttpIntegration { payloadExample payloadAlertFields { type path label } } } } } }
I can run this precise query today and it is completely valid against our current schema.
Edited by Alex Kalderimis
assigned to @ck3g and unassigned @alexkalderimis
changed milestone to %13.11
added missed-deliverable missed:13.10 labels
2 3 module Resolvers 4 module AlertManagement 5 class HttpIntegrationResolver < BaseResolver 6 alias_method :project, :synchronized_object 7 8 type Types::AlertManagement::HttpIntegrationType, null: true 9 10 argument :id, Types::GlobalIDType[::AlertManagement::HttpIntegration], 11 required: true, 12 description: 'ID of the integration.' 13 14 def resolve(**args) 15 return unless Ability.allowed?(current_user, :admin_operations, project) 16 17 GitlabSchema.object_from_id(args[:id], expected_class: ::AlertManagement::HttpIntegration) mentioned in merge request !57590 (merged)
Closing in favour of !57590 (merged)
mentioned in issue #326295 (closed)