Update `XrayReport` model JSON schema & remove `xray_reports.file_checksum` column

The following discussion from !162313 (merged) should be addressed:

• @mikolaj_wawrzyniak started a discussion: (+2 comments)

  nitpick: Have you considered extracting version string into a const to create SSoT from the start?

  @lma-git: I originally just hardcoded this number in because it will likely be temporary. If we proceed with deprecating Repository X-Ray, we would remove scannerVersion from the payload entirely.

  But extracting it to a const will clean it up and allow me to explain further in a comment. So I'll go ahead and do that.

Considerations

• Do we really need checksums anymore? See #479185 (comment 2140346559).

Proposal

• Update the XrayReport JSON schema:
  • Remove scannerVersion and checksum.
  • Change fileName to file_path.
  • Remove/update the fields above in Ai::Context::Dependencies::ConfigFiles::Base and Ai::RepositoryXray::ScanDependenciesService.
• Remove the xray_reports.file_checksum database column.
  • NOTE: This will likely require a multi-step release approach (docs reference).
  • Related issues:
    • #500462 (closed)
    • #500465 (closed)

### Proposal update Due to the database-level constraints caused by the column file_checksum not being nullable, we will encounter issues when we remove checksum from the XRay Report schema as per the proposal above, including failing tests. Therefore, we're adding an additional step to add a migration that allows file_checksum to be nullable before removing the field from the Xray Report schema.

Follow up

As previously mentioned above, removing the xray_reports.file_checksum column requires a multi-milestone approach. This issue can be closed as complete in %17.6. Follow-up issues for the remaining removal steps are:

• (%17.7) Dropping the column (release M+1)
• (%17.8) Removing the ignore rule (release M+2)

added groupcode creation typemaintenance labels

assigned to @lma-git

added to epic &14100

added Category:Code Suggestions devopscreate sectiondev labels

changed the description

added backend label

changed title from Follow-up from "Reproduce Repository X-Ray functionality - Introduce lock file classes" to {+Update XrayReport model JSON schema+}

changed the description

marked this issue as blocked by #483928 (closed)

added workflowblocked label

changed the description

Hi @ck3g @mikolaj_wawrzyniak, I have a question about the XrayReport model that you might have insight into.

Currently the JSON schema for the payload column is:

{
  "definitions": {
    "scannerVersion": {
      "type": "string"
    },
    "fileName": {
      "type": "string"
    },
    "checksum": {
      "type": "string"
    },
    "libs": {
      "type": "array",
      "items": {
        "type": "object"
      }
    }
  }
}

Do you know why we anticipated needing the checksum value? It doesn't seem to be used anywhere for verification from what I can see so far.

With the new in-monolith parsing service implemented in #476177 (closed), I'm thinking we can just remove the checksum value all together and also remove the dedicated column xray_reports.file_checksum. The main reason is because I plan to merge the dependencies extracted from multiple files into a single xray report (#491800 (closed)), so it doesn't quite make sense to have a checksum for a single or multiple files, especially if they're not necessary.

Are there any concerns with this?

@mnohr Perhaps you might remember why we had checksum in the xray reports? See question in #479185 (comment 2138774414)

@lma-git afair it was envisioned as a way to verify if new scan is required, I am not sure if it ever got implemented though.

Edit: At the begging scan deduplication was delegated to CI job trigger rules. As for removal, do not have strong opinion around deduplication strategies, apart from the fact that it is required. So if you have other solution in mind, imo feel free to remove artefacts from the deprecated solution and add more suitable ones instead.

Yes. The idea was to check whether the dependency file has been changed since last scan. But that logic wasn't implemented

@mikolaj_wawrzyniak @ck3g Thank you both! I see

With the new in-monolith service, the cost of parsing the file is relatively low compared to the Gitaly calls we make to pull the files in the first place. So I don't think it would be that beneficial to implement the checksum logic at this time. However, we could re-consider it in the future if it becomes necessary.

I think we can proceed with removing the checksum fields until we have a more concrete strategy moving forward (particularly one that supports multiple files in a single X-ray report).

The new service has a Sidekiq deduplication strategy and exclusive lease so that only one X-ray Sidekiq job runs at a time per project. This already significantly reduces how often we're running the service, so I don't see re-parsing the same files as a concern at this time.

Thank you for your input!

I think we can proceed with removing the checksum fields until we have a more concrete strategy moving forward (particularly one that supports multiple files in a single X-ray report).

hi @lma-git

I'm currently picking up this issue and just wanted to check in and clarify that, as per above, we still want to go ahead and remove the checksum field from the schema in Repository-X-Ray and in the monolith?

Hi @squadri Correct. The checksum field can be removed from the payload schema. We should also remove the file_checksum column which will need a database migration. Make sure to refer to the documentation with regards to removing columns: https://docs.gitlab.com/ee/development/database/avoiding_downtime_in_migrations.html#dropping-columns.

@lma-git great, thanks for confirming. One other question:

Proposal

• Update the XrayReport JSON schema:
  • Change fileName to file_path.

I am assuming we are changing fileName to filePath for readability purposes or is there another reason I should probably be aware of?

@squadri

we are changing fileName to filePath for readability purposes

It's mostly for consistency IMO. I use the term file_paths in Ai::RepositoryXray::ScanDependenciesService, so the payload should follow suit. "Path" is more descriptive of its value. And in general, snake case is preferred over camel case in field names.

@squadri Luckily the JSON schema for payload isn't strict nor enforced by the database, so I included small changes like fileName -> file_path because they're easy to implement. Unlike the column removal, the JSON schema changes and references to its fields can all be done in one MR.

@lma-git Great, thanks for the explanation.

mentioned in merge request !167843 (merged)

changed the description

mentioned in issue create-stage#13229 (closed)

changed title from Update XrayReport model JSON schema to Update XrayReport model JSON schema{+ & remove xray_reports.file_checksum column+}

changed the description

added workflowready for development label and removed workflowblocked label

removed the relation with #483928 (closed)

unassigned @lma-git

mentioned in merge request !168728 (merged)

changed milestone to %17.6

mentioned in issue gitlab-org/quality/triage-reports#20244 (closed)

added code-creation-prioritymedium label

set weight to 2

assigned to @squadri

added workflowin dev label and removed workflowready for development label

mentioned in merge request gitlab-org/code-creation/repository-x-ray!85 (closed)

mentioned in merge request !169751 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20326 (closed)

mentioned in merge request !169920 (merged)

changed the description

changed the description

mentioned in issue #500462 (closed)

mentioned in issue #500465 (closed)

changed the description

mentioned in issue gitlab-org/quality/triage-reports#20442 (closed)

mentioned in merge request !170936 (merged)

added workflowverification label and removed workflowin dev label

added workflowin review label and removed workflowverification label

Async Status Update 2024-11-01

Current Status:

• I have merged two MRs to update the XrayReport JSON schema and add an ignore_column rule and a migration to make the file_checksum column nullable as part of the multi-step approach to remove the xray_reports.file_checksum database column.

Next Steps:

As per the multi-step release guidance, we are required to drop the column and remove the ignore_column rule in two separate milestones. I have created two separate follow-up issues to address that:

• [#500462 (closed)] Drop file_checksum column from xray_reports table - %17.7
• [#500465 (closed)] Remove ignore_column rule for file_checksum in xray_report model - %17.8

Blockers: N/A

Shipping this milestone? No, due to the requirement of a multi-step release approach across three separate milestones for DB migration related work. Therefore, I'll keep this issue open until all subsequent issues are resolved.

changed the description

Closed with !169920 (merged) (merged) and !170936 (merged) (merged).

closed

Changing to workflowcomplete based on the engineering workflow

added workflowcomplete label and removed workflowin review label

mentioned in merge request !173116 (merged)

mentioned in merge request !176891 (merged)