Enum name for sort fields should be taken from Column.key instead of Column.name

[davidcim]

I'm working with an external Microsoft SQL database and some field names contain invalid characters such as %. Fortunately, the parameter key of SQLAlchemy Column allows you to specify a different name for a field to be used everywhere except in the SQL code:

key – An optional string identifier which will identify this Column object on the Table. When a key is provided, this is the only identifier referencing the Column within the application, including ORM attribute mapping; the name field is used only when rendering SQL.

Usually the key attribute in the orm column is the same as the name attribute. But, as in this instance, if you specify the key parameter when defining the column:

descuento1 = Column("% descuento1", Numeric, key="descuento1")

... sort_enum_for_object_type function will generate the name of the sort enum from the original SQL field name instead of from the provided key alternative as it would be expected. As a result, the generated enum name is wrong and an exception is thrown:

AssertionError: Names must match /^[_a-zA-Z][_a-zA-Z0-9]*$/ but "%_DESCUENTO1_ASC" does not.

This PR just makes the name to be taken from the key attribute which is usually identical except if an alternative field name is specified. Everything seems to work fine in my tests and I can't find any side effects resulting from the change.

tox -e pre-commit:

Check for merge conflicts................................................Passed
Check Yaml...............................................................Passed
Debug Statements (Python)................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
flake8...................................................................Passed
seed isort known_third_party.............................................Passed
isort....................................................................Passed

[coveralls]

Coverage remained the same at 97.611% when pulling c821bce on davidcim:master into 20ecaea on graphql-python:master.

[codecov-commenter]

Codecov Report

Merging #301 (957df3d) into master (a47dbb3) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #301   +/-   ##
=======================================
  Coverage   96.39%   96.39%           
=======================================
  Files           9        9           
  Lines         721      721           
=======================================
  Hits          695      695           
  Misses         26       26           

Impacted Files	Coverage Δ
graphene_sqlalchemy/enums.py	97.80% <100.00%> (ø)

---

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 a47dbb3...957df3d. Read the comment docs.

[erikwrede]

Thanks for the PR; I just checked it out!
I agree with your points. The current behavior is intransparent because it can cause sort enum names to differ from the field's names in the schema.
However, since the rest of the library uses the field names instead of the column names, wouldn't it make more sense to use the field name here instead of the column.key to handle naming uniformly everywhere?
Functionally, I don't think it makes a difference since column.key seems to be initialized to the field name instead of column.name if no custom key is provided.
It's just a matter of handling names the same way in the entire codebase to prevent future deviations like this.
I must admit, it's a bit of nitpicking, but I'd be happy to have your opinion on this.

I've also found another case where column.name is used:

graphene-sqlalchemy/graphene_sqlalchemy/converter.py

Lines 241 to 249 in a47dbb3

	@convert_sqlalchemy_type.register(ChoiceType)
	def convert_choice_to_enum(type, column, registry=None):
	name = "{}_{}".format(column.table.name, column.name).upper()
	if isinstance(type.type_impl, EnumTypeImpl):
	# type.choices may be Enum/IntEnum, in ChoiceType both presented as EnumMeta
	# do not use from_enum here because we can have more than one enum column in table
	return Enum(name, list((v.name, v.value) for v in type.choices))
	else:
	return Enum(name, type.choices)

It looks like that could cause the same errors, so it might be worth fixing as well.

Although this is a breaking change, I think merging this is the way to go. My comments from above might be worth discussing, but I don't believe changing something is a hard requirement.

Can you add a test case for this?

[erikwrede]

Added to #353, thank you for the PR!

[davidcim]

Erik,
Sorry I didn't reply sooner, I'm swamped with work. If I have time I will make a new PR with the changes you suggest and tests.
Regards.

[erikwrede]

Hey David, no worries! I added your changes to #353, including unit tests, so everything is done 🙂

If you find the time, maybe check out the changes in the corresponding commit and let me know if that works for you.

Thanks again for bringing this up!

[erikwrede]

Commit in PR is: dccbe22