Skip to content

Conversation

@jorroll
Copy link
Contributor

@jorroll jorroll commented Sep 18, 2017

New feature:

This pull adds an ActiveNode class method self.additional_mapped_label_names= that maps additional labels to a model.

Before this patch

The only way you can add multiple labels to a model is through inheritance or with module mixins (though I didn't actually realize the module option existed until I was writing the specs for this pull request).

After this patch

class User
  include Neo4j::ActiveNode
  self.additional_mapped_label_names = ['Author', 'Admin']

  class Person < User
  end
end

user = User.create
user.labels
#=> [:User, :Author, :Admin]

person = Person.create
person.labels
#=> [:Person, :User, :Author, :Admin]

There are multiple ways a feature like this could be implemented. The way I've implemented it leaves mapped_label_name, and the existing DSL, untouched.

Each model still needs a single mapped_label_name and the mapped_label_name requires a constraint. additional_mapped_label_names are in addition to the mapped_label_name, and they do not require constraints.

The need

In my app, I have several models that need multiple labels such as Role::Template::Current (which has labels Role, Current, Role::Current, Role::Template, Role::Template::Current. Currently, I need to jump through some hoops to get there...

Pings:
@cheerfulstoic
@subvertallchris

@jorroll jorroll changed the title add: optional additional_mapped_label_names for ActiveNodes add: additional_mapped_label_names option to ActiveNode Sep 18, 2017
@jorroll
Copy link
Contributor Author

jorroll commented Sep 18, 2017

I figure folks might have some opinions on if / how this feature is implemented. If this pull is accepted, I can update the docs to reference the new option.

@codecov-io
Copy link

codecov-io commented Sep 18, 2017

Codecov Report

Merging #1414 into master will increase coverage by 0.02%.
The diff coverage is 98.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1414      +/-   ##
==========================================
+ Coverage    96.9%   96.93%   +0.02%     
==========================================
  Files         205      205              
  Lines       12417    12501      +84     
==========================================
+ Hits        12033    12118      +85     
+ Misses        384      383       -1
Impacted Files Coverage Δ
spec/unit/active_node/labels_spec.rb 100% <100%> (ø) ⬆️
lib/neo4j/active_node/labels.rb 92.59% <91.66%> (-0.2%) ⬇️
spec/integration/label_spec.rb 98.16% <92.85%> (-0.79%) ⬇️
lib/neo4j/tasks/migration.rake 32.14% <0%> (+0.83%) ⬆️

Continue to review full report at Codecov.

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

Copy link
Contributor

@cheerfulstoic cheerfulstoic left a comment

Choose a reason for hiding this comment

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

Just a very small comment on this so far. For a long time I feel like there's been a shortcoming in the gem with dealing with more complex cross-cutting models and I haven't really dug into finding a good way to solve it, so I want to think about this deeper when I have the time to dedicate to it.

when String then names = [given_names]
else
fail '"additional_mapped_label_names" must be a string or array of strings'
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could do:

names = case given_names
        when Array then given_names
        when String then [given_names]
        else
          fail '"additional_mapped_label_names" must be a string or array of strings'
        end

@jorroll
Copy link
Contributor Author

jorroll commented Sep 19, 2017

@cheerfulstoic, updated formatting

@jorroll
Copy link
Contributor Author

jorroll commented Sep 19, 2017

My thinking with additional_mapped_label_names is to basically treat them like inherited labels. The mapped_label_name is the, theoretically unique, label which identifies a particular model. The additional_mapped_label_names only come into play at the times when inherited mapped_label_names would.

@jorroll
Copy link
Contributor Author

jorroll commented Sep 23, 2017

While this PR still seems like a valuable feature to have, I think #1424 provides a better solution to my particular problem, and would largely negate my need for this.

@leehericks
Copy link

The inheritance-based label system currently in place is very understandable. It also allows for a separation of logic code, as an Admin class can inherit from User and have admin specific logic kept separate from the basic User class.

class User
  include Neo4j::ActiveNode
  self.additional_mapped_label_names = ['Author', 'Admin']

  class Person < User
  end
end

I understand it's example code, but when are all users by default Authors and Admins?

So I agree with Brian that some other way of implementing cross-cutting concerns should be investigated because inheritance isn't reusable.

Neo4j is pretty flexible in how you choose to model the data. I'm not an authoritative source, but I find this GraphAware article to be an interesting look at the use of multiple labels.

In my app, I have several models that need multiple labels such as Role::Template::Current (which has labels Role, Current, Role::Current, Role::Template, Role::Template::Current.

Without more information, I can't fully understand why you are relying on so many combination of labels logically. The above article shows incredible performance hits for matching multiple labels or using negation.

All that said, labels are indeed an integral part of the the Neo4j property graph model. If possible, there should be a labels property and the ability to add labels and call save to persist them. Then before a save you could apply labels based on some logic, you could also mix in a module with shared logic based on labels?

@cheerfulstoic
Copy link
Contributor

Thanks @leehericks Definitely check out #1424 because we have conversation going on about some possibilities over there. I'd be curious to know what you think about the discussion / proposals

@klobuczek klobuczek deleted the branch neo4jrb:master January 1, 2024 19:22
@klobuczek klobuczek closed this Jan 1, 2024
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

Successfully merging this pull request may close these issues.

5 participants