Skip to content

Commit 1726d1c

Browse files
Sukuna0007AbhiSukuna0007Abhi
authored
Factor out common code to address issue #11
- Created new common package to house shared utilities - Analyzed code duplication across comid and corim packages - Extracted TaggedURI type to common package - Updated comid/entity.go to use common.TaggedURI via type alias - Maintained backward compatibility with existing code - Added comprehensive documentation in common/README.md - All tests pass successfully Fixes #11 Signed-off-by: Sukuna0007Abhi <[email protected]>
1 parent 35b2a5f commit 1726d1c

File tree

6 files changed

+197
-6
lines changed

6 files changed

+197
-6
lines changed

‎REFACTORING_NOTES.md‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
# Issue #11: Factor out common code
2+
3+
## Summary
4+
5+
This PR addresses issue #11 by:
6+
7+
1. **Analyzing** the codebase for duplicated code across packages
8+
2. **Creating** the `common` package to house shared utilities
9+
3. **Documenting** findings and recommendations in `common/README.md`
10+
4. **Extracting** `TaggedURI` as the first common utility
11+
12+
## Changes Made
13+
14+
### New Files
15+
- `common/doc.go` - Package documentation
16+
- `common/README.md` - Comprehensive analysis of code duplication
17+
- `common/tagged_uri.go` - Extracted TaggedURI type
18+
19+
### Modified Files
20+
- `comid/entity.go` - Now uses `common.TaggedURI` via type alias
21+
- `comid/comid.go` - Updated comments for String2URI function
22+
23+
## Analysis Results
24+
25+
The analysis identified three main areas of duplication:
26+
27+
1. **EntityName Implementation** (~200 lines duplicated between comid and corim)
28+
- Complex due to CBOR tag registry differences
29+
- Recommendation: Keep as-is or phase 2 refactoring
30+
31+
2. **Role/Roles Implementation** (~150 lines duplicated)
32+
- Different role constants per package
33+
- Recommendation: Possible future extraction with registry pattern
34+
35+
3. **TaggedURI** (~10 lines) ✅ **EXTRACTED**
36+
- Simple utility type with no dependencies
37+
- Successfully moved to common package
38+
39+
## Testing
40+
41+
All existing tests continue to pass:
42+
- ✅ comid package tests
43+
- ✅ corim package tests
44+
- ✅ coev package tests
45+
- ✅ cots package tests
46+
- ✅ encoding package tests
47+
- ✅ extensions package tests
48+
- ✅ coserv package tests
49+
50+
## Backward Compatibility
51+
52+
All changes maintain backward compatibility through type aliases:
53+
- `comid.TaggedURI` is now an alias for `common.TaggedURI`
54+
- Existing code using `comid.TaggedURI` continues to work unchanged
55+
56+
## Future Work
57+
58+
As documented in `common/README.md`, potential future refactoring could include:
59+
60+
- **Phase 2**: Extract Role/Roles base types with a registry pattern
61+
- **Phase 3**: Consider EntityName extraction if a clean solution for CBOR tag handling emerges
62+
63+
The analysis shows that further extraction requires careful consideration of:
64+
- CBOR tag registry differences between packages
65+
- Maintaining backward compatibility
66+
- Avoiding circular dependencies
67+
- Cost/benefit of added complexity vs. code deduplication
68+
69+
## Notes
70+
71+
- The original issue mentioned `cocli/cmd/common.go` print functions, but cocli has been moved to a separate repository
72+
- This PR takes a conservative, incremental approach to refactoring
73+
- The `common/README.md` serves as documentation for future refactoring efforts

‎comid/comid.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ func IsAbsoluteURI(s string) error {
107107
return nil
108108
}
109109

110+
// String2URI converts a string pointer to a TaggedURI pointer.
111+
// Returns nil if the input is nil. Validates that the string is an absolute URI.
110112
func String2URI(s *string) (*TaggedURI, error) {
111113
if s == nil {
112114
return nil, nil
@@ -119,7 +121,6 @@ func String2URI(s *string) (*TaggedURI, error) {
119121
v := TaggedURI(*s)
120122

121123
return &v, nil
122-
123124
}
124125

125126
// AddEntity adds an organizational entity, together with the roles this entity

‎comid/entity.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"fmt"
1010
"unicode/utf8"
1111

12+
"github.com/veraison/corim/common"
1213
"github.com/veraison/corim/encoding"
1314
"github.com/veraison/corim/extensions"
1415
)
@@ -333,8 +334,5 @@ func RegisterEntityNameType(tag uint64, factory IEntityNameFactory) error {
333334
return nil
334335
}
335336

336-
type TaggedURI string
337-
338-
func (o TaggedURI) Empty() bool {
339-
return o == ""
340-
}
337+
// TaggedURI is a type alias for common.TaggedURI for backward compatibility.
338+
type TaggedURI = common.TaggedURI

‎common/README.md‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
# Common Package
2+
3+
This package contains shared types and utilities used across the `corim`, `comid`, and related packages.
4+
5+
## Purpose
6+
7+
Created to address issue #11: "Factor out common code"
8+
9+
## Analysis of Common Code
10+
11+
After analyzing the codebase, the following areas of duplication were identified:
12+
13+
### 1. Entity Name Implementation
14+
- **Location**: `comid/entity.go` and `corim/entity.go`
15+
- **Duplication**:
16+
- `EntityName` struct and methods (~200 lines duplicated)
17+
- `StringEntityName` type and methods
18+
- `IEntityNameValue` interface
19+
- `IEntityNameFactory` type
20+
- Factory functions: `NewEntityName`, `MustNewEntityName`, `NewStringEntityName`, `MustNewStringEntityName`
21+
- CBOR/JSON marshaling logic for EntityName
22+
- Entity name registry and `RegisterEntityNameType`
23+
24+
**Recommendation**: The EntityName implementation is nearly identical between comid and corim packages. However, each package requires different CBOR tag registries. A refactoring would need to:
25+
- Extract common base types to `common` package
26+
- Keep package-specific CBOR/JSON marshaling in each package
27+
- Or use a more sophisticated registry pattern
28+
29+
### 2. Role/Roles Implementation
30+
- **Location**: `comid/role.go` and `corim/role.go`
31+
- **Duplication**:
32+
- `Role` type (int64)
33+
- `Roles` slice type
34+
- `RegisterRole` function
35+
- `String()` method
36+
- `Valid()` method for Roles
37+
- JSON marshaling/unmarshaling logic
38+
- roleToString and stringToRole maps
39+
40+
**Differences**:
41+
- Different role constants (comid: TagCreator, Creator, Maintainer; corim: ManifestCreator)
42+
- comid's Roles.Add() doesn't validate, corim's does
43+
- corim has additional ToJSON/FromJSON helper methods
44+
- comid has ToCBOR/FromCBOR methods
45+
46+
**Recommendation**: Could extract a base `Role` type and registration system, but each package would keep its own role constants and any package-specific behavior.
47+
48+
### 3. TaggedURI
49+
- **Location**: `comid/entity.go`
50+
- **Usage**: Used by both comid and corim Entity types
51+
- Simple string wrapper with `Empty()` method
52+
53+
**Recommendation**: Easy candidate for extraction - it's a simple utility type with no dependencies.
54+
55+
### 4. Common Validation/Marshaling Patterns
56+
- Many types implement similar `Valid()`, `MarshalCBOR()`, `UnmarshalCBOR()`, `MarshalJSON()`, `UnmarshalJSON()` patterns
57+
- Uses `encoding.PopulateStructFromCBOR`, `encoding.SerializeStructToCBOR`, etc.
58+
59+
**Recommendation**: These patterns are already well-factored through the `encoding` package. No further action needed.
60+
61+
## Implementation Considerations
62+
63+
### Challenges
64+
1. **CBOR Tag Registration**: comid and corim have separate CBOR tag registries. Moving types to `common` would require either:
65+
- Maintaining separate encoders/decoders in each package
66+
- Creating a more complex registry system
67+
- Keeping marshaling logic in each package
68+
69+
2. **Package Dependencies**: Need to avoid circular dependencies between common, comid, and corim
70+
71+
3. **Backward Compatibility**: Any refactoring must maintain the existing public API
72+
73+
4. **Test Coverage**: Extensive test suites exist for current code - all must continue to pass
74+
75+
### Recommended Approach
76+
77+
Given the complexity of CBOR tag handling and the relatively small size of the codebase, the most practical approach is:
78+
79+
1. **Phase 1** (this PR): Document common code patterns and create the `common` package structure
80+
2. **Phase 2** (future PR): Extract truly independent utilities (like TaggedURI)
81+
3. **Phase 3** (future PR): Consider more complex refactoring of Role/EntityName if warranted
82+
83+
## Status
84+
85+
**Current**: Analysis and documentation complete.
86+
**Next Steps**: Team decision on whether to proceed with extraction or keep as-is with documentation.
87+
88+
Note: The `cocli` package mentioned in the original issue has been moved to a separate repository, so cocli/cmd/common.go is no longer relevant to this codebase.

‎common/doc.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
// Copyright 2021-2024 Contributors to the Veraison project.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
// Package common contains shared types and utilities used across the corim,
5+
// comid, and related packages. This package was created to factor out common
6+
// code as per issue #11.
7+
package common

‎common/tagged_uri.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// Copyright 2021-2024 Contributors to the Veraison project.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package common
5+
6+
// TaggedURI represents a URI string. It is used by both comid and corim
7+
// packages for entity registration IDs.
8+
type TaggedURI string
9+
10+
// Empty returns true if the TaggedURI is an empty string.
11+
func (o TaggedURI) Empty() bool {
12+
return o == ""
13+
}
14+
15+
// String2URI converts a string pointer to a TaggedURI pointer.
16+
// Returns nil if the input is nil or empty.
17+
func String2URI(s *string) (*TaggedURI, error) {
18+
if s == nil || *s == "" {
19+
return nil, nil
20+
}
21+
22+
uri := TaggedURI(*s)
23+
return &uri, nil
24+
}

0 commit comments

Comments
 (0)