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

transform_rel_type to upcase leaving lowercased relations in database. #1491

Closed phyllisstein closed 6 years ago

phyllisstein commented 6 years ago

Hey folks! Just wanted to flag a glitch I noticed with the transform_rel_type configuration option. In my application.rb, I've set the following config flag:

config.neo4j.transform_rel_type = :upcase

...and I've added relations like these to my Neo4j::ActiveNode classes:

has_one :out, :root_node, type: :first_child, model_class: 'Content::Node'

...but in the database itself, the edges are still labeled with lowercase names, as input, without the transformation:

graph 1

It's possible that I'm missing something, but it seems as though the transformations aren't being applied. Any idea what might be going on? Thanks in advance for any guidance you can provide.

Additional information which could be helpful if relevant to your issue:

Code example (inline, gist, or repo)

(See above.)

Runtime information:

Neo4j database version: neo4j gem version: 9.1.5 neo4j-core gem version: 8.1.1

cheerfulstoic commented 6 years ago

Thanks for the report! As a sanity check: Were the relationships already in the database when you made the change to the code, or were they created after? If they were already there you'll need a migration to change them.

Another thought, though: I think that the transform_rel_type was created before we required either type, rel_class, or origin on an association. When we introduced that change, it forced users to specify their type manually. So if you're using the type option and you want it to be upper-case then you would need to make the type argument value upper case. For associations this line is the place where we "decorate" the default type, but now that we require one of those three options I don't think it can ever get to that final else. So I think that means that this configuration only applies to ActiveRel currently (I could use a check from @subvertallchris on that, though)

If that seems reasonable to you, I could change the docs to explain this. Also happy to have a larger conversation if you think that it's needed.

phyllisstein commented 6 years ago

Sanity-wise, these relationships were created after I added the transform_rel_type flag. What you're describing certainly makes sense, though! I don't think there's a pressing need to make the docs any more explicit; I'd sort of just made an assumption that didn't bear out. Thanks so much for the clarification, and for the stellar work on this gem.

cheerfulstoic commented 6 years ago

I made a simple change anyway to clarify, particularly because it talks about associations, which is just not right. I try to work under the assumption that if somebody makes an issue about something that there was or will be 1-2 people with the same issue who don't ;)

Thanks again!

subvertallchris commented 6 years ago

It's been a while since I thought about this feature but everything @cheerfulstoic wrote also sounds correct to me. Comments in the Neo4j::Shared::RelTypeConverters module corroborate both the hunch and what you're seeing.

Sorry for the confusion, @phyllisstein, and thanks for checking it out!