cast enum value to string

[wwmoraes]

As per https://graphql.org/graphql-js/type/#graphqlenumtype, enum values can be of any type, often integers, thus requiring a cast to string type to trim/replace/split. Without this cast, integer values raise a TypeError: value.trim is not a function.

As per GraphQL docs:

class GraphQLEnumType {
  constructor(config: GraphQLEnumTypeConfig)
}

type GraphQLEnumTypeConfig = {
  name: string;
  values: GraphQLEnumValueConfigMap;
  description?: ?string;
}

type GraphQLEnumValueConfigMap = {
  [valueName: string]: GraphQLEnumValueConfig;
};

type GraphQLEnumValueConfig = {
  value?: any;
  deprecationReason?: string;
  description?: ?string;
}

type GraphQLEnumValueDefinition = {
  name: string;
  value?: any;
  deprecationReason?: string;
  description?: ?string;

GraphQL serializes Enum values as strings, however internally Enums can be represented by any kind of type, often integers.

Example given by the docs:

var RGBType = new GraphQLEnumType({
  name: 'RGB',
  values: {
    RED: { value: 0 },
    GREEN: { value: 1 },
    BLUE: { value: 2 }
  }
});

[mickhansen]

What does your Sequelize model look like? I don't think we should toString if typeof is number, just return it directly, should be no need to sanitize.

[wwmoraes]

@mickhansen FWIW my model uses an Enum pretty much like the example (I just have different names).

Sanitization would still be needed if we use a float or another unusual type for value. Since ECMA 1st, toString() is the way all objects expose their string representation; that said, it wouldn't be needed and I'd agree with you if the GQL spec said, instead of value?: any, value?: string | number.

Nonetheless...

GraphQL serializes Enum values as strings.

... so adding a cast to string and forcing sanitization for all types is even compliant with GQL serialization.

[mickhansen]

@wwmoraes But this code converts from a sequelize type to a graphql type so you'd actually have an ENUM defined in your database with numeric values right?

[wwmoraes]

@wwmoraes But this code converts from a sequelize type to a graphql type so you'd actually have an ENUM defined in your database with numeric values right?

TL;DR Only PostgreSQL supports enum, and even there it works with string values effectively, so imo I don't see a problem there.

Details Internally PostgreSQL creates an OID for the Enum itself, and stores for each enum value, in pg_enum catalog, a 4-byte float to indicate the value order and a 64-byte case-sensitive string for the label, which is used on queries (but it does internal relationship using the pg_enum OID of the value row).

[mickhansen]

@wwmoraes I think you are missing what i'm asking. You must have a Sequelize schema with numeric enums, right?

[wwmoraes]

@wwmoraes I think you are missing what i'm asking. You must have a Sequelize schema with numeric enums, right?

Perhaps. Indeed my sequelize enum schema is made of numeric values, but that's my preference, not a must. What's your point?

I've checked sequelize queryGenerator and it does pass forward the type as-is, i.e.: MyModel.create({ rgb: 1 }), executes INSERTO INTO <MyModelName> ([...],´rgb`) VALUES ([...],1); MyModel.create({ rgb: '1' }), executes INSERTO INTO <MyModelName> ([...],´rgb`) VALUES ([...],'1'); MyModel.create({ rgb: RGBType.GREEN }), executes INSERTO INTO <MyModelName> ([...],´rgb`) VALUES ([...],1);

Is that where you're aiming at?

PS.: even if I change my values to a string, function sanitizeEnumValue() does change '1' to '_1'. Am I missing something on the DBMS'/GraphQL API docs that justifies this underscore?

[mickhansen]

I was mostly curious if the values came back as numbers aswell from Sequelize when you query for them.

even if I change my values to a string, function sanitizeEnumValue() does change '1' to '_1'. Am I missing something on the DBMS'/GraphQL API docs that justifies this underscore?

Not sure, i believe that was based on the spec for enum names, but i didn't write that code.

Regardless, i don't see a reason to call toString if typeof is number, but that's not so important. Mind adding a unit test so there's no an accidental regression later?

[wwmoraes]

I was mostly curious if the values came back as numbers aswell from Sequelize when you query for them.

I've run some tests on a console and sequelize casts and also returns as a string:

> MyModel.create({ rgb: 1 });
(…)
> MyModel.findOne({ attributes: ['rgb'] });
Promise {
  MyModel {
    dataValues:
     { type: '1' },
(…)
> MyModel.findOne({ where: { rgb: 1 }, attributes: ['rgb'] });
Promise {
  MyModel {
    dataValues:
     { type: '1' },
(…)
}

Regardless, i don't see a reason to call toString if typeof is number, but that's not so important.

Agreed, it seems that function sanitizeEnumValue() needs deeper rewrite, as it has some odd sanitization as I've stated before (i.e. '1' becomes '_1'). Mind if I raise an issue?

I'll close the pull and change my values to be alpha-only strings for the time being.

[mickhansen]

@wwmoraes Please do raise an issue, and will try and figure out who wrote that code initially so they can chime in