Skip to content

Conversation

@cuixq
Copy link
Contributor

@cuixq cuixq commented Oct 28, 2025

Unblocks #4190

This PR updates vulnfeeds to use the latest version of github.com/ossf/osv-schema/bindings/go.

@cuixq
Copy link
Contributor Author

cuixq commented Oct 28, 2025

/gemini summary
/gemini review

@gemini-code-assist
Copy link
Contributor

Hello @cuixq,

Thank you for submitting this pull request! I've reviewed the changes and generated a summary and a detailed review below.

Pull Request Summary

This pull request migrates the vulnfeeds module to use the latest version of github.com/ossf/osv-schema/bindings/go. This update involves significant changes to align with the new protobuf-based schema bindings, which primarily affects how OSV vulnerability objects are structured and accessed.

Key changes include:

  • Dependency Update: The go.mod and go.sum files have been updated to reflect the new osv-schema/bindings/go version and introduce google.golang.org/protobuf.
  • Field Access Migration: Direct access to OSV schema fields (e.g., v.ID, v.Affected, r.URL) has been replaced with corresponding getter methods (e.g., v.GetId(), v.GetAffected(), r.GetUrl()).
  • Timestamp Handling: time.Time fields for Published and Modified are now converted to timestamppb.New(time.Time) to align with the timestamppb.Timestamp type in the new schema.
  • Structured Data Conversion: map[string]any fields, such as DatabaseSpecific and EcosystemSpecific, have been converted to structpb.Struct for compatibility with protobuf. This required introducing helper functions (eventsToStructValueManual, rangesToStructValueManual) to manually convert slices of osvschema.Event and osvschema.Range into structpb.Value for storage within structpb.Struct fields.
  • Enum Type Adoption: String-based reference and severity types (e.g., osvschema.ReferenceFix, osvschema.SeverityCVSSV3) have been updated to their new enum counterparts (e.g., osvschema.Reference_FIX, osvschema.Severity_CVSS_V3).
  • Test Updates: Test files have been adjusted to use protocmp.Transform() for accurate comparison of protobuf messages and to reflect the new data structures and access patterns.

This migration is essential for unblocking https://github.com/google/osv.dev/pull/4190 and ensures vulnfeeds is compatible with the latest OSV schema.

Code Review

Overall, the changes in this pull request represent a thorough and well-executed migration to the new OSV schema bindings. The adoption of getter methods, timestamppb.Timestamp, structpb.Struct, and enum types is consistent with protobuf best practices and significantly improves type safety and maintainability.

General Feedback

  • Correctness (Critical): All instances of direct field access have been correctly updated to use getter methods. The conversion of time.Time to timestamppb.Timestamp and map[string]any to structpb.Struct appears to be handled correctly across the codebase, including the necessary manual conversion logic for nested structures.
  • Completeness (High): The migration seems comprehensive, covering all affected files and ensuring consistency with the new schema. The go.mod and go.sum updates are also correctly applied.
  • Maintainability (Medium): The transition to protobuf-native types and getter methods enhances the long-term maintainability and type safety of the codebase. The introduction of manual conversion functions for DatabaseSpecific and EcosystemSpecific is a pragmatic solution to handle the schema change for these flexible fields.
  • Testing (High): The test cases have been appropriately updated to reflect the new schema and leverage protocmp.Transform() for accurate comparisons, which is crucial for verifying the correctness of the migration.

No specific issues were found that require immediate action. The changes are primarily mechanical and correctly implemented to adapt to the new schema bindings.

@cuixq cuixq marked this pull request as ready for review October 28, 2025 02:57
Comment on lines +107 to +111
cweList := make([]*structpb.Value, 0, len(cwes))
for _, cwe := range cwes {
cweList = append(cweList, structpb.NewStringValue(cwe))
}
v.DatabaseSpecific.Fields["cwe_ids"] = structpb.NewListValue(&structpb.ListValue{Values: cweList})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate this 😭 Are we going to have to do this for every field we want to add to database_specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably factor out to a helper function that takes the key and the list of strings to construct the struct, so we don't need to repeat this every time.

do we expect to see more variation of fields in database_specific?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be neater to call ds := v.DatabaseSpecific.AsMap() at the beginning, then v.DatabaseSpecific, err = structpb.NewStruct(ds) at the end of these functions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, use NewValue(cwes) instead of NewXValue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that at the very least I also want to add the programFiles info to database_specific

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants