activegraph icon indicating copy to clipboard operation
activegraph copied to clipboard

ActiveNode property and scope with same name cause issue

Open ziaahmad opened this issue 4 years ago • 7 comments

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

ziaahmad avatar Feb 06 '20 13:02 ziaahmad

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

klobuczek avatar Feb 06 '20 14:02 klobuczek

@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.

ziaahmad avatar Feb 07 '20 08:02 ziaahmad

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.

amitsuryavanshi avatar Feb 07 '20 08:02 amitsuryavanshi

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

ziaahmad avatar Feb 07 '20 09:02 ziaahmad

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.

amitsuryavanshi avatar Feb 07 '20 10:02 amitsuryavanshi

(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.

weedySeaDragon avatar Feb 07 '20 21:02 weedySeaDragon

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

amitsuryavanshi avatar Feb 08 '20 06:02 amitsuryavanshi