api-platform / admin

A beautiful and fully-featured administration interface builder for hypermedia APIs
https://api-platform.com/docs/admin/
MIT License
480 stars 130 forks source link

feat: enum input guesser #479

Closed PawelSuwinski closed 1 year ago

PawelSuwinski commented 1 year ago

My proposition for enum input guesser.

Closes #477

alanpoulain commented 1 year ago

I've added a commit with my changes, WDYT?

PawelSuwinski commented 1 year ago

I've added a commit with my changes, WDYT?

I think that now it looks better :). Putting humanizing to api-doc-parser allows to use an inflector, that is already present in deps and not to try to do it manually by some hardcoded regexp especially that it is not trivial thing, there are a lot of unusual cases.

Anyway at the end IMHO parser+guesser should cover humanizing out of the box two kinds of typically used enums: (1) CONSTANT_CASE - it is already done here (and even more thanks to using inflector) by openapi2/3 api-doc-parser and (2) schema.org likes enumerations - usually its camel case (or rather camel caps) prefixed with namespace, for example:

'https://schema.org/AudiobookFormat' => {"Audiobook format":  "https://schema.org/AudiobookFormat" }

So with this approach (humanize in api-doc-parser) second case is for future hydra api-doc-parser implementation.

For now last thing to do I think is to take advantage of humanized enums or transformEnum extra prop in FieldGuesser.

PawelSuwinski commented 1 year ago

For now last thing to do I think is to take advantage of humanized enums or transformEnum extra prop in FieldGuesser.

I think that it could be done this way, untested draft:

// FieldGuesser
(...)
  if (field.enum) {
    const renderEnum =
      (props as FunctionFieldProps).render ??
      ((record) =>
        Object.entries(field.enum ?? {}).find(
          ([, v]) => v === props.source,
        )?.[0] ?? (props.source ? record[props.source] : null));

    return (
      <FunctionField {...(props as FunctionFieldProps)} render={renderEnum} />
    );
  }

And similar (primaryText prop) in case of ArrayFiled.

alanpoulain commented 1 year ago

Anyway at the end IMHO parser+guesser should cover humanizing out of the box two kinds of typically used enums: (1) CONSTANT_CASE - it is already done here (and even more thanks to using inflector) by openapi2/3 api-doc-parser and (2) schema.org likes enumerations - usually its camel case (or rather camel caps) prefixed with namespace, for example:

'https://schema.org/AudiobookFormat' => {"Audiobook format":  "https://schema.org/AudiobookFormat" }

So with this approach (humanize in api-doc-parser) second case is for future hydra api-doc-parser implementation.

Do you have an example where schema.org values need to be used for enums?

Why not for the FieldGuesser :slightly_smiling_face: You should create an EnumField dedicated component though.

PawelSuwinski commented 1 year ago

Do you have an example where schema.org values need to be used for enums?

API Platform at that moment supports only schema.org Enumeration as enums AFAIK, so to have complete solution (at least on api level) one needs to use schema.org built-in types for a field range (for example https://schema.org/BookFormatType) or use own dictionary (my case) with classes that extends https://schema.org/Enumeration:

https://github.com/api-platform/schema-generator/blob/ab36e6dadc7e316ad10afcc99a2b15e0dbb43552/src/Schema/Model/Class_.php#L23

In that case generator generate Assert\Choice constraint for such field with enums values (full name with the namespace prefix): https://github.com/api-platform/schema-generator/blob/ab36e6dadc7e316ad10afcc99a2b15e0dbb43552/src/AttributeGenerator/ConstraintAttributeGenerator.php#L74

And at the and as you wrote last it goes as enum in api doc: https://github.com/api-platform/admin/issues/477#issuecomment-1263313614

I am using here my custom decorator to do it and suppress namespace but without it enum list will contain fully qualified values like 'https://schema.org/EBook'.

Why not for the FieldGuesser slightly_smiling_face You should create an EnumField dedicated component though.

FunctionField seems to have all that is needed for job to be done, is dedicated EnumField than really needed?

I meant something like this:

diff --git a/src/FieldGuesser.tsx b/src/FieldGuesser.tsx
index dd5b159..c6b93ea 100644
--- a/src/FieldGuesser.tsx
+++ b/src/FieldGuesser.tsx
@@ -6,6 +6,7 @@ import {
   ChipField,
   DateField,
   EmailField,
+  FunctionField,
   NumberField,
   ReferenceArrayField,
   ReferenceField,
@@ -20,6 +21,7 @@ import type {
   BooleanFieldProps,
   DateFieldProps,
   EmailFieldProps,
+  FunctionFieldProps,
   NumberFieldProps,
   ReferenceArrayFieldProps,
   ReferenceFieldProps,
@@ -89,6 +91,19 @@ const renderField = (

   const fieldType = schemaAnalyzer.getFieldType(field);

+  if (field.enum) {
+    const renderEnum =
+      (props as FunctionFieldProps).render ??
+      ((record) =>
+        Object.entries(field.enum ?? {}).find(
+          ([, v]) => v === props.source,
+        )?.[0] ?? (props.source ? record[props.source] : null));
+
+    return (
+      <FunctionField {...(props as FunctionFieldProps)} render={renderEnum} />
+    );
+  }
+
   switch (fieldType) {
     case 'email':
       return <EmailField {...(props as EmailFieldProps)} />;
alanpoulain commented 1 year ago

Alright, I was reading this enum (generated from OpenAPI) and not this one (Hydra).

Still, I am really not sure it's a good idea to have a regexp to handle schema.org or any other RDF vocabulary like this. I think we can postpone this until we have a fully JSON-LD/Hydra enum support in API Platform.

FunctionField seems to have all that is needed for job to be done, is dedicated EnumField than really needed?

Yes I think it will be better: your renderEnum function is not trivial.

PawelSuwinski commented 1 year ago

Still, I am really not sure it's a good idea to have a regexp to handle schema.org or any other RDF vocabulary like this.

I agree. I rather thought about cutting off the name space and just humanize the rest value, something like this: inflection.humanize(enumValue.replace(namespace,'')).

I think we can postpone this until we have a fully JSON-LD/Hydra enum support in API Platform.

OK, its just future considerations.

FunctionField seems to have all that is needed for job to be done, is dedicated EnumField than really needed?

Yes I think it will be better: your renderEnum function is not trivial.

OK, I will think about it and propose something in the next PR if it is still not solved to not too block this one and delay merging .

alanpoulain commented 1 year ago

I agree. I rather thought about cutting off the name space and just humanize the rest value, something like this: inflection.humanize(enumValue.replace(namespace,'')).

Yes it would be better!

alanpoulain commented 1 year ago

Thank you @PawelSuwinski :slightly_smiling_face: