neo4jrb / activegraph

An active model wrapper for the Neo4j Graph Database for Ruby.
http://neo4jrb.io
MIT License
1.4k stars 276 forks source link

ActiveNode property and scope with same name cause issue #1588

Open ziaahmad opened 4 years ago

ziaahmad commented 4 years ago

Code example (inline, gist, or repo)

class Category
  include Neo4j::ActiveNode

  id_property :uid, on: :next_id
  property :name, type: String
  property :description, type: String
  property :main, type: Boolean

  scope :main, -> { where(main: true) }

  def next_id
    self.class.query_as(:c).pluck('MAX(c.uid)').first.to_i + 1
  end
end

Runtime information:

Category.first returns #<Category uid: 1, description: "", main: #<QueryProxy []>, name: "Paraplyer">

And Category.main results in infinite loop of calling "main" scope for every object, which results in "Stack level too deep" error

Neo4j database version: neo4j gem version: 9.6.1 neo4j-core gem version: 9.0.0

klobuczek commented 4 years ago

@ziaahmad are you suggesting that same names should be allowed or that other error should be raised?

ziaahmad commented 4 years ago

@klobuczek I think same names should be allowed. Like if I replace scope with a class method, as given below

class Category
  include Neo4j::ActiveNode

  id_property :uid, on: :next_id
  property :name, type: String
  property :description, type: String
  property :main, type: Boolean

  def self.main
    where(main: true)
  end

  def next_id
    self.class.query_as(:c).pluck('MAX(c.uid)').first.to_i + 1
  end
end

it starts working without any issues. Then why can't we implement it as scope.

amitsuryavanshi commented 4 years ago

I assume this has to be with the feature that, when you define a scope, you get an instance method with the same scope name as well.

ziaahmad commented 4 years ago

I didn't get this. Why it creates an instance method for every scope. Scope is meant to be treated as Class method.

amitsuryavanshi commented 4 years ago

In absence of it, one would need to do, user.as(:n).active_friends, which is redundant. With instance method one can just do user.active_friends.

weedySeaDragon commented 4 years ago

(jumping in) In Rails, scope is a macro that essentially resolves to a class method. Implementing scope as an instance method (e.g. as in https://github.com/neo4jrb/neo4j/blob/0ce207eccd8fe12db7c5bf7fd1f6cd8e0dfef1c9/lib/neo4j/active_node/scope.rb#L37 is wrong/incorrect.

@amitsuryavanshi your example of user.active_friends. is not correct (as far a Rails is concerned): User.active_friends is not assumed to produce the same result as when you call it on an instance of User: some_user.active_friends

Forgive me if you already know that. But that code in active_node is obv. the problem and my opinion is that you should not be including that instance method. If people want to have the instance method, they can easily write code to delegate an instance method to the class method (scope).

But providing it as you have means that people who happen to be working with a graph with a property name the same as a scope will have to do some gymnastics.

amitsuryavanshi commented 4 years ago

@weedySeaDragon Thanks for your feedback. We will take this into consideration.