Add namespace to pins

[awrichar]

Existing pins will all be categorized against "ff_system", just so the field has a non-null value.

[codecov-commenter]

Codecov Report

Merging #856 (2d5a320) into main (eb61c4c) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #856   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          299       299           
  Lines        19388     19379    -9     
=========================================
- Hits         19388     19379    -9     

Impacted Files	Coverage Δ
internal/apiserver/routes.go	100.00% <ø> (ø)
internal/orchestrator/orchestrator.go	100.00% <ø> (ø)
pkg/core/pin.go	100.00% <ø> (ø)
internal/apiserver/route_get_pins.go	100.00% <100.00%> (ø)
internal/database/sqlcommon/pin_sql.go	100.00% <100.00%> (ø)
internal/events/batch_pin_complete.go	100.00% <100.00%> (ø)
internal/orchestrator/data_query.go	100.00% <100.00%> (ø)
internal/orchestrator/persistence_events.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb61c4c...2d5a320. Read the comment docs.

[peterbroadhurst]

Code looks great to me. I did have a couple of questions for you to ponder whether there's more to do before this merges.

[peterbroadhurst]

I've added a migration_consideration tag, as we've chosen ff_system really only because it's a reserved namespace name that can be used to query these values back (definitely not because these pre-upgrade pins were all for the ff_system namespace).

So we should document that.

[awrichar]

Yea, originally I was going to leave it blank but then realized the API would give you no way to query them (and that seemed bad).

Pins were treated as "global" in 1.0, and the aggregator loop that processes pins even logs them as if they are part of ff_system (see here), so it didn't feel terribly wrong to pick ff_system for this purpose. But it was always a bit inaccurate, as they are specific to a namespace even if they weren't treated that way.

Anyway, bit of a ramble but I agree we need to document it.

[peterbroadhurst]

Do you think we should we update the indexes to have namespace as a dimension?

I've noticed we did that elsewhere when we introduced a namespace, and I guess it should help with isolation of performance considerations between namespaces in the case of a shared DB.

[peterbroadhurst]

As part of answering this, I think it would be good to look at this code:

firefly/internal/events/aggregator.go

Lines 224 to 232 in eb61c4c

	func (ag *aggregator) getPins(ctx context.Context, filter database.Filter, offset int64) ([]core.LocallySequenced, error) {
	log.L(ctx).Tracef("Reading page of pins > %d (first pin would be %d)", offset, offset+1)
	pins, _, err := ag.database.GetPins(ctx, filter)
	ls := make([]core.LocallySequenced, len(pins))
	for i, p := range pins {
	ls[i] = p
	}
	return ls, err
	}

If the event aggregator is to become namespace aware, then I would expect the filter passed into this function by the parent to include a namespace filter. I'm not sure if that's true today.

[awrichar]

Good catch, and I think this should be rolled into #855 where the aggregator becomes namespace-aware.

[peterbroadhurst]

Can we do that without introducing yet another migration file?

[awrichar]

Yes, can do. We've said migrations aren't guaranteed to be stable in mainline (only between releases).

[awrichar]

See here

[peterbroadhurst]

Also need to cover the moving of the API in the migration docs

[peterbroadhurst]

Added approval - note my request to see if when #855 merges we can re-use the migration file, rather than adding a new one.