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

QueryProxy replace_with compares IDs globally, instead of per model-class #1653

Open
yourpalal opened this issue Apr 5, 2021 · 2 comments
Open

Comments

@yourpalal
Copy link

yourpalal commented Apr 5, 2021

While investigating #1652, I noticed that QueryProxy replace_with compares IDs for all models to determine which relationships to delete and insert.

Assuming the id in question is a uuid, this probably works out fine, but when using multiple model classes, there could be some overlap of ids between the two model classes.

Note: the problem in the example below is currently hidden by the bug reported in #1652 . If pluck(:id) is were fixed, but no other changes were applied, then #1652 could be considered fixed, while this issue would not be.

Probably the best course of action is to fix this and #1652 at the same time.

Code example (inline, gist, or repo)

class Book
  include ActiveGraph::Node
  
  id_property :isbn
  
  has_one :in, :publisher, origin: :published_things, model_class: %w(Publisher)
end

class Magazine
  include ActiveGraph::Node
  
  id_property :issue_id
  
  has_one :in, :publisher, origin: :published_things, model_class: %w(Publisher)
end

class Publisher
  include ActiveGraph::Node
  
  has_many :out, :published_things, type: :published, model_class: %w(Book Magazine)
end

original_zine, book = [ Magazine.create!(issue_id: "1234"), Book.create!(isbn: "098512=25") ]
publisher = Publisher.create!
publisher.published_things = [original_zine, book]

replacement_zine = Magazine.create!(issue_id: "098512=25") # same as book isbn
publisher.published_things = [replacement_zine] # leads to replace_with being called

# replacement_zine did not get a rel, since it has the same id as book
replacement_zine.reload
puts replacement.publisher # puts nil

# book was not removed, since it has the same id as replacement_zine
book.reload
puts book.publisher # puts non-nil

Runtime information:

Neo4j database version:
neo4j gem version: 10.1.0
neo4j-ruby-driver gem version: 1.7.5

@klobuczek
Copy link
Member

@yourpalal do you currently have overlapping ids between models? I know that requiring global uniqueness is unnecessarily strong, but it is best practice. What id generator are you using?

@yourpalal
Copy link
Author

We do have some overlapping ids between models. We import various external datasets and use the IDs used in those datasets as id_properties. Often, one data set will have several data types, which then become different models in activegraph.

In some cases, there is overlap of IDs within a single dataset, and in others there is overlap between these datasets.

Many datasets we import use integer ids (probably AUTOINT in their database) which can increase the likelihood of overlap between datasets.


Generally, any non-uuid ID property could end up running into this issue. If universally-unique IDs are required by ActiveGraph, that would be pretty easy to achieve by always using a uuid property, but I don't believe this is stated anywhere in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants