Replace Job's untyped hashes with T::ImmutableStruct#14616
Replace Job's untyped hashes with T::ImmutableStruct#14616JamieMagee wants to merge 1 commit intomainfrom
Conversation
…s T::Hash[String, T.untyped] with T::ImmutableStruct for the three main untyped collections in the Job class: - AllowedUpdate: dependency_name, dependency_type, update_type, update_types fields replace hash key lookups like update.fetch("dependency-name") - IgnoreCondition (Job-scoped): dependency_name, version_requirement, update_types, source fields replace ic["dependency-name"] etc. - SecurityAdvisoryEntry: dependency_name, affected_versions, patched_versions, unaffected_versions replace adv["affected-versions"] Each struct has a from_hash class method that parses the raw JSON hash at the boundary (in the Job constructor). Downstream code gets typed field access instead of T.untyped hash lookups.
There was a problem hiding this comment.
Pull request overview
This PR refactors Dependabot::Job to replace three untyped “bag of keys” hashes (allowed_updates, ignore_conditions, security_advisories) with typed T::ImmutableStruct models, parsing raw JSON once at construction time and using typed field access downstream.
Changes:
- Introduces
Dependabot::Job::AllowedUpdate,Dependabot::Job::IgnoreCondition, andDependabot::Job::SecurityAdvisoryEntryimmutable structs withfrom_hashparsing. - Updates
Dependabot::Jobto expose typed arrays forallowed_updates,ignore_conditions, andsecurity_advisoriesand migrates internal accessors accordingly. - Updates
DependencyChange#pr_messageto convert typed ignore conditions back into the hash format expected byDependabot::PullRequestCreator::MessageBuilder.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| updater/lib/dependabot/job/allowed_update.rb | Adds typed model for allowed_updates, including a temporary to_hash bridge. |
| updater/lib/dependabot/job/ignore_condition.rb | Adds typed model for job ignore conditions parsed from raw JSON. |
| updater/lib/dependabot/job/security_advisory_entry.rb | Adds typed model for job security advisories parsed from raw JSON. |
| updater/lib/dependabot/job.rb | Converts job fields to typed structs and updates matching/filtering logic to use typed accessors. |
| updater/lib/dependabot/dependency_change.rb | Converts typed ignore conditions back to hashes when calling MessageBuilder. |
| sig { returns(T::Array[Dependabot::Job::SecurityAdvisoryEntry]) } | ||
| attr_reader :security_advisories |
There was a problem hiding this comment.
security_advisories is now typed as T::Array[SecurityAdvisoryEntry], but there are still call sites treating elements as hash-like (e.g., updater/lib/dependabot/updater/operations/refresh_security_update_pull_request.rb uses job.security_advisories.first["dependency-name"]). That will raise at runtime because SecurityAdvisoryEntry doesn’t implement #[]. Update remaining callers to use the struct’s typed accessors (e.g., job.security_advisories.first&.dependency_name) instead of hash indexing / T.cast.
| .select { |adv| T.must(adv.dependency_name.casecmp(dependency.name)).zero? } | ||
|
|
||
| relevant_advisories.map do |adv| | ||
| vulnerable_versions = adv["affected-versions"] || [] | ||
| safe_versions = (adv["patched-versions"] || []) + | ||
| (adv["unaffected-versions"] || []) | ||
|
|
||
| Dependabot::SecurityAdvisory.new( | ||
| dependency_name: dependency.name, | ||
| package_manager: package_manager, | ||
| vulnerable_versions: vulnerable_versions, | ||
| safe_versions: safe_versions | ||
| vulnerable_versions: T.unsafe(adv.affected_versions), | ||
| safe_versions: T.unsafe(adv.patched_versions + adv.unaffected_versions) | ||
| ) |
There was a problem hiding this comment.
security_advisories_for is currently using T.unsafe to pass arrays of version strings into Dependabot::SecurityAdvisory. This undermines the goal of eliminating T.untyped propagation and can hide invalid data until later. Prefer converting adv.affected_versions into the expected Dependabot::Requirement objects (using the requirement class for package_manager) and pass safe_versions as a normal T::Array[String] (which already matches SecurityAdvisory’s safe_versions type) so you can remove the T.unsafe calls entirely.
| const :dependency_name, String | ||
| const :version_requirement, T.nilable(String) | ||
| const :update_types, T.nilable(T::Array[String]) | ||
| const :source, T.nilable(String) | ||
|
|
||
| sig { params(hash: T::Hash[String, T.untyped]).returns(IgnoreCondition) } | ||
| def self.from_hash(hash) | ||
| new( | ||
| dependency_name: hash.fetch("dependency-name"), | ||
| version_requirement: hash["version-requirement"], | ||
| update_types: hash["update-types"], | ||
| source: hash["source"] | ||
| ) |
There was a problem hiding this comment.
IgnoreCondition drops fields that other parts of the system can use when rendering ignore-condition tables (notably "updated-at", which Dependabot::PullRequestCreator::MessageBuilder uses to sort “Most Recent Ignore Conditions”). Consider adding an updated_at (or similar) field to this struct and parsing it in from_hash, so downstream code (e.g., DependencyChange#pr_message) can preserve ordering semantics instead of all entries being treated as the epoch (Time.at(0)).
| { | ||
| "dependency-name" => ic.dependency_name, | ||
| "version-requirement" => ic.version_requirement, | ||
| "source" => ic.source |
There was a problem hiding this comment.
When passing ignore conditions into Dependabot::PullRequestCreator::MessageBuilder, the mapping currently omits "updated-at". MessageBuilder#ignore_conditions_table uses that field to sort and show the latest 20 ignore conditions; without it, all entries sort as Time.at(0) and the “most recent” table becomes misleading. Preserve and forward updated-at (and any other fields MessageBuilder depends on) from Job::IgnoreCondition when building this hash payload.
| "source" => ic.source | |
| "source" => ic.source, | |
| "updated-at" => ic.updated_at |
What are you trying to accomplish?
allowed_updates,ignore_conditions, andsecurity_advisoriesin theJobclass wereT::Hash[String, T.untyped]bags. Every access likeupdate.fetch("dependency-name")returnedT.untyped, which propagated through every method that touched them.This replaces the three hashes with
T::ImmutableStructmodels (AllowedUpdate,Job::IgnoreCondition,SecurityAdvisoryEntry). Each has afrom_hashclass method that parses the raw JSON in theJobconstructor. After that, all downstream code uses typed field access (update.dependency_name) instead of string key lookups.Follows the "parse, don't validate" approach from #13139, applied to updater.
Anything you want to highlight for special attention from reviewers?
The
Job::IgnoreConditionstruct is separate fromDependabot::Config::IgnoreCondition(which lives in common). The Job version includes thesourcefield used for logging, which the Config version doesn't have.calculate_update_configmaps between them.dependency_change.rb converts back to hashes when passing ignore conditions to
MessageBuilder, since that class lives in common and shouldn't depend on updater types.How will you know you've accomplished your goal?
bundle exec srb tcpasses with no errors.bundle exec rubocopis clean. The struct fields are now type checked at compile time, so any typo in a field name (e.g.update.dependecy_name) would fail the type checker instead of silently returning nil at runtime.