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

fix: Prevent multiple docs from being linked in one one #1790

Merged

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #1646

Description

Prevents multiple docs from being linked in one one.

It does this by scanning if from primary, or a point-lookup if from secondary. By going via the Fetcher interface and newFetcher func this should respect Lens, and any indexes once the are implemented.

A handful of import/export tests got removed as they would no longer have anything to do with import/export once corrected (the error occurs before that action).

@AndrewSisley AndrewSisley added area/query Related to the query component action/no-benchmark Skips the action that runs the benchmark. labels Aug 15, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.7 milestone Aug 15, 2023
@AndrewSisley AndrewSisley requested a review from a team August 15, 2023 19:57
@AndrewSisley AndrewSisley self-assigned this Aug 15, 2023
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Patch coverage: 52.25% and project coverage change: -0.19% ⚠️

Comparison is base (a1270b1) 75.88% compared to head (6793ab9) 75.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1790      +/-   ##
===========================================
- Coverage    75.88%   75.69%   -0.19%     
===========================================
  Files          209      209              
  Lines        22106    22213     +107     
===========================================
+ Hits         16774    16812      +38     
- Misses        4183     4233      +50     
- Partials      1149     1168      +19     
Flag Coverage Δ
all-tests 75.69% <52.25%> (-0.19%) ⬇️

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

Files Changed Coverage Δ
db/collection_update.go 72.87% <46.43%> (-3.26%) ⬇️
db/collection.go 71.87% <50.00%> (-1.84%) ⬇️
db/errors.go 81.13% <100.00%> (+0.51%) ⬆️

... and 5 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 a1270b1...6793ab9. Read the comment docs.

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.

LGTM

@AndrewSisley AndrewSisley merged commit 1cb993d into sourcenetwork:develop Aug 15, 2023
12 checks passed
@AndrewSisley AndrewSisley deleted the 1646-one-one-acts-as-one-many branch August 15, 2023 22:27
@@ -600,3 +602,12 @@ func NewErrDocCreate(inner error) error {
func NewErrDocUpdate(inner error) error {
return errors.Wrap(errDocUpdate, inner)
}

func NewErrOneOneAlreadyLinked(documentId, targetId, relationName string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: rename to documentID, targetID

@jsimnz
Copy link
Member

jsimnz commented Aug 16, 2023

This is a concerningly inefficient approach here. This should very much be refactored once #298 is completed, where each relation gets its own unique index.

This would be analgeous to an SQL foreign key unique constraint. It will ofc be a space/time tradeoff, indexes using more space, but will turn this operation into an O(1) instead of O(n), at the cost of O(n) space.

This tradeoff can be exposed via the @relation directive at the time of schema creating by the dev/user if they wish to preserve space.

Eg.

type Author {
  name: String
  book: Book @relation(index: false) # by default this would be (index: true)
}

@fredcarle
Copy link
Collaborator

really@@hi

shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…k#1790)

## Relevant issue(s)

Resolves sourcenetwork#1646

## Description

Prevents multiple docs from being linked in one one.

It does this by scanning if from primary, or a point-lookup if from
secondary. By going via the `Fetcher` interface and `newFetcher` func
this should respect Lens, and any indexes once the are implemented.

A handful of import/export tests got removed as they would no longer
have anything to do with import/export once corrected (the error occurs
before that action).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/query Related to the query component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Updating one-to-one relation link makes it act like a one-to-many
4 participants