Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow mutation of col sources via PatchCollection #2424

Merged

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #2422

Description

Allows the mutation of collection sources via PatchCollection.

Users can now set view and migration Lens transforms, and update view queries via json patch.

Validation is non-existant RE the Query struct, as it is in develop branch. An invalid config will result in errors when querying the view. I think we can improve on this later, this PR does however increase the likelihood of invalid view queries being constructed (by users).

It also massively increases the exposure of the client/request package, which whilst already public, is quite poorly documented. Similar to validation, this PR increases the priority of documenting it.

@AndrewSisley AndrewSisley added feature New feature or request area/collections Related to the collections system labels Mar 18, 2024
@AndrewSisley AndrewSisley added this to the DefraDB v0.11 milestone Mar 18, 2024
@AndrewSisley AndrewSisley requested a review from a team March 18, 2024 21:20
@AndrewSisley AndrewSisley self-assigned this Mar 18, 2024
@AndrewSisley AndrewSisley changed the title feat: Allow mutation of collection sources via PatchCollection feat: Allow mutation of col sources via PatchCollection Mar 18, 2024
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 78.94737% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 74.85%. Comparing base (839b505) to head (2fae5b5).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2424      +/-   ##
===========================================
- Coverage    74.85%   74.85%   -0.00%     
===========================================
  Files          268      268              
  Lines        25875    25928      +53     
===========================================
+ Hits         19367    19406      +39     
- Misses        5183     5192       +9     
- Partials      1325     1330       +5     
Flag Coverage Δ
all-tests 74.85% <78.95%> (-<0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
db/errors.go 65.13% <100.00%> (+0.82%) ⬆️
db/collection.go 73.08% <75.00%> (+0.03%) ⬆️

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

Comment on lines +705 to +726
newColSources := newCol.CollectionSources()
oldColSources := oldCol.CollectionSources()

if len(newColSources) != len(oldColSources) {
return NewErrCollectionSourcesCannotBeAddedRemoved(newCol.ID)
}

for i := range newColSources {
if newColSources[i].SourceCollectionID != oldColSources[i].SourceCollectionID {
return NewErrCollectionSourceIDMutated(
newCol.ID,
newColSources[i].SourceCollectionID,
oldColSources[i].SourceCollectionID,
)
}
}

newQuerySources := newCol.QuerySources()
oldQuerySources := oldCol.QuerySources()

if len(newQuerySources) != len(oldQuerySources) {
return NewErrCollectionSourcesCannotBeAddedRemoved(newCol.ID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo: This validation is no longer doing what it was doing before. This means that the name is no longer accurate. Sources can be modified. We can't add or remove them or change their ID, be they can be modified. Maybe change the name to validateSourcesModification.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 20, 2024

Choose a reason for hiding this comment

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

Good spot, will do.

  • Rename validation func

Comment on lines 91 to 92
"firstName": "John",
"lastName": "S"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: If you don't mind removing the extra spaces that would be great.

Copy link
Contributor Author

@AndrewSisley AndrewSisley Mar 20, 2024

Choose a reason for hiding this comment

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

A sign that I'm too reliant on my IDE to do this for me...

  • Fix spacing

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

Approving assuming the minor todo will be resolved. The tests are really nice in helping to visualize what's happening.

@AndrewSisley AndrewSisley merged commit 75cf5ad into sourcenetwork:develop Mar 20, 2024
31 of 32 checks passed
@AndrewSisley AndrewSisley deleted the 2422-patch-col-migrations branch March 20, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/collections Related to the collections system feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow mutation of collection sources via PatchCollection
2 participants