I feel a bit uncomfortable with this implementation, but not so much that I'd block it

Yeah I feel this too 😆 - I really wasn't sure how to do this optimally/reusably...

So I think the bigger picture idea is that you can have an OSCAL model (this is just for component definition, but really any OSCAL Model) that can have links to other resources (remote or local). If we want to sort of package up all this data we may need to re-write the paths. Additionally, in the case of a component definition, it can import other component definitions (which contain these links) and those internal references will need to be re-written when the data gets imported to the importer model (ala, what we do with Compose right now, although those don't resolve paths, that function just puts all validations in the backmatter, which is not great for some of the use cases we've discussed with the SSP, etc).

The initial thought on this was to brute force look through all the spots where you'd possibly need to change a reference, and just do it. Although you can see from the list of items in pathsToRemap that it's a little on the long side. So totally agreed that reflect is not optimal, but that was my solution to not brute forcing it. Maybe there's another option that I'm overlooking if you have an idea on how to do it differently, I'm all ears!

My discomfort is because this feels like an implementation for iterating over raw json, but we have concrete types, so I'd expect something that iterated over each type.

Here's some probably-awful pseudo code:

func (c ComponentDefinition) RewritePaths (paths, anythingelse) error {
for r := range c.Resources {
if r.Href != "" {
rewritePath(r.Href)
}
for s := range c.OtherField {
// something like this for everything
}
}

Though for the actual implementation (and this is where my handwavey future ideas come into play, so again THIS MIGHT NOT MAKE SENSE for right now/ever) I'd probably have each type with links (every field inside the ComponentDefinition) fulfill an interface that includes a RewritePaths method, so we could handle each separately.

Ok fair enough, yeah agreed maybe it would be better to just write a super long function and have all the types checked by type and not with reflect. This does get a little tedious as well because of all the pointers... To be honest, this is probably something I could coerce chat-gipity to give me so I didn't have to write it manually 🤣

For the future idea, I truly don't think I know enough about go-oscal to know if that's do-able. But maybe worth investigating more...

I suppose for the near term we're probably trading the two options: using the reflect implementation or write a big function. (selfishly/lazily I do think this implementation is easier to test/understand)

@brandtkeller if you have any weigh-ins on this TIA!

Reading back through the issue and then this implementation - I am weighing the "what problem does this solve now" with "are the surrounding assumptions correct".

Yes the solutions complexity obfuscates some of the operations - I had to spend some time with debugging to get a clear-ish picture.

But we have an original problem statement - composing component definitions that import other component definitions and remapping paths. Relevant problem for us - as each imported component-definition has potential links to Lula Validations or potentially other artifacts (which is why I appreciated the breadth of support in this PR).

Apologies in advance. I believe we see the intent of a package operation for sake of transport - which allows us to dictate the artifact retrieval and replacement for re-mapping (evidence for a solution).

Do we see ourselves having any mechanism of getting away from a compose like operation? for component-definitions and allowing the import of other component-defintions - it feels like the answer is no and that this will be tied intimately to the execution of validate functionality - whether directly with Lula or with LaaL. Gut feeling here is that i'd default to a verbose solution that is easily traceable over a minimal solution that is less traceable.

@brandtkeller @mildwonkey - Updated this PR to remove the reflect method (as that sounded broadly unpopular 😅 ) in favor of the verbose model walk/rewrite in place

I'm a bit of a mess - this is all notional work not tied into execution currently - do we have issue(s) that add this to current execution?

#896 may be the execution implementation?

#896 may be the execution implementation

That's right - trying to decide if that PR should also include instrumenting the method in the generate SSP function or if that should be separate. But that's really the endgame here: enable the SSP having the right links to pass through to the Assessment Plan (so then we can do a thing with that).

Alright, I know you all already approved this but I think @mildwonkey was right about refactoring that huge function (ultimately I do see reuse across models, so the goal was to make that more reusable/clear/testable) - with that, I decided to not be lazy and made the following updates:

• Broke down the ComponentDefinition RewritePaths method into smaller functions (that I put in common.go because they aren't all specific to the component definition model (I didn't break out every type, the intent was to break into types that I saw reused and/or ones that were nested to facilitate testing)
• Updated the test files for ^^ method - honestly it seemed unnecessarily confusing with the abs paths in there and hopefully the point still stands. I think import/validate will use real paths but mock paths I believe are sufficient to test this. After going through the above exercise, I also wanted to address gaps in testing all the elements of ComponentDefinition
• Added a fuzz to RemapPath, hopefully it's a valid implementation 😅