CROSSLINK-201 LMS config directory #375
Conversation
customData still the same type. directory.Entries is generated from the directory OpenAPI spec.
adamdickmeiss
requested review from
JanisSaldabols and
jakub-id
as code owners
adamdickmeiss
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors the LMS (Library Management System) configuration in the Directory service by renaming the "ncip" field to "lmsConfig" and restructuring the configuration schema. The changes move from a map-based configuration approach to a strongly-typed OpenAPI schema with generated Go types.
Changes:
- Renamed and restructured the LMS configuration from "ncip" to "lmsConfig" in the OpenAPI schema with camelCase naming
- Refactored LMS adapter to use generated
directory.LmsConfigtype instead ofmap[string]any - Updated all references and tests to use the new configuration structure
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| directory/directory_api.yaml | Renamed "Ncip" schema to "LmsConfig", changed field names from snake_case to camelCase, and updated field types and defaults |
| broker/lms/lms_adapter_ncip.go | Refactored to use directory.LmsConfig type, removed manual parsing logic, simplified configuration handling |
| broker/lms/lms_creator_impl.go | Added JSON marshaling/unmarshaling to convert CustomData to directory.Entry type |
| broker/lms/lms_adapter_test.go | Removed configuration parsing tests, updated existing tests to use new directory.LmsConfig structure |
| broker/lms/lms_creator_test.go | Updated test data to use "LmsConfig" instead of "ncip" field name |
| broker/ncipclient/ncipclient.go | Removed unused constant definitions that were moved/refactored |
| broker/adapter/api_directory.go | Updated to use generated directory.EntriesResponse and directory.Entry types with proper field access |
| BibliographicRecordIdentifierCode: &ncip.SchemeValuePair{Text: *l.config.RequestItemBibIdCode}, | ||
| }} | ||
| requestScopeTypeField := ncip.SchemeValuePair{Text: l.requestItemRequestScopeType} | ||
| requestScopeTypeField := ncip.SchemeValuePair{Text: *l.config.RequestItemRequestScopeType} | ||
|
|
||
| requestTypeField := ncip.SchemeValuePair{Text: l.requestItemRequestType} | ||
| requestTypeField := ncip.SchemeValuePair{Text: *l.config.RequestItemRequestType} |
There was a problem hiding this comment.
Potential nil pointer dereference. The fields RequestItemBibIdCode, RequestItemRequestScopeType, and RequestItemRequestType are dereferenced without nil checks. According to the OpenAPI schema, these fields have defaults ("SYSNUMBER", "Item", "Page") but are not marked as required, meaning they could be nil. The code should either check for nil and use defaults, or ensure these fields are always populated during initialization.
|
|
||
| func (l *LmsAdapterNcip) InstitutionalPatron(requesterSymbol string) string { | ||
| return strings.ReplaceAll(l.institutionalPatron, "{symbol}", strings.ToUpper(requesterSymbol)) | ||
| return strings.ReplaceAll(*l.config.InstitutionalPatron, "{symbol}", strings.ToUpper(requesterSymbol)) |
There was a problem hiding this comment.
Potential nil pointer dereference. The field InstitutionalPatron is dereferenced without a nil check. According to the OpenAPI schema, this field is not marked as required and has no default value, meaning it could be nil. The code should check for nil before dereferencing to avoid a panic.
| return strings.ReplaceAll(*l.config.InstitutionalPatron, "{symbol}", strings.ToUpper(requesterSymbol)) | |
| if l.config.InstitutionalPatron == nil { | |
| return "" | |
| } | |
| pattern := *l.config.InstitutionalPatron | |
| return strings.ReplaceAll(pattern, "{symbol}", strings.ToUpper(requesterSymbol)) |
| return nil, err | ||
| if l.config.Address == "" { | ||
| return nil, fmt.Errorf("missing NCIP address in LMS configuration") | ||
| } |
There was a problem hiding this comment.
Missing required field validation. According to the OpenAPI schema, FromAgency is a required field for LmsConfig, but there's no validation here to ensure it's not empty. The code should validate that l.config.FromAgency is not empty before using it.
| } | |
| } | |
| if strings.TrimSpace(l.config.FromAgency) == "" { | |
| return nil, fmt.Errorf("missing NCIP FromAgency in LMS configuration") | |
| } |
| - disabled | ||
| - auto | ||
| - manual | ||
| description: Base URL of the the LMS API |
There was a problem hiding this comment.
Typo in description: "the the" should be "the".
| description: Base URL of the the LMS API | |
| description: Base URL of the LMS API |
| if err == nil { | ||
| err = json.Unmarshal(dataBytes, &customData) | ||
| if err != nil { |
There was a problem hiding this comment.
Silent error handling. If json.Marshal(d) fails at line 74, the error is silently ignored and customData remains nil. This could lead to unexpected behavior downstream. Consider either logging the error or handling it more explicitly, especially since this is converting data that should be valid JSON (as it was just unmarshaled).
| if err == nil { | |
| err = json.Unmarshal(dataBytes, &customData) | |
| if err != nil { | |
| if err != nil { | |
| // Ensure CustomData is always a usable map even if marshaling fails. | |
| customData = map[string]any{} | |
| } else { | |
| if err = json.Unmarshal(dataBytes, &customData); err != nil { |
| $ref: '#/components/schemas/Tier' | ||
| ncip: | ||
| lmsConfig: | ||
| type: object |
There was a problem hiding this comment.
Redundant type declaration. The line "type: object" is redundant when using $ref. The $ref already indicates the schema type, so the type declaration should be removed.
| type: object |
| lmsConfig: | ||
| type: object | ||
| $ref: '#/components/schemas/Ncip' | ||
| Ncip: | ||
| $ref: '#/components/schemas/LmsConfig' |
There was a problem hiding this comment.
Breaking API change. The field name in the Entry schema has been renamed from "ncip" to "lmsConfig". This is a breaking change that will affect any existing directory data that uses the "ncip" field name. Existing directory entries will need to be migrated, or the code needs to handle backward compatibility by checking for both field names during a transition period.
No description provided.