Skip to content

Array fields #87

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bertramakers opened this issue Nov 6, 2023 · 8 comments
Closed

Array fields #87

bertramakers opened this issue Nov 6, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@bertramakers
Copy link
Contributor

Hi @tobyzerner ! Today I was wondering how to define a field of a specific type like Str, Integer, Date, ... that accepts multiple values.

In JSON Schema it could be described as this for e.g. an array of 1-10 values, each one a string from 1-255 characters:

{
  "type": "object",
  "properties": {
    "tags": {
      "type": "array",
      "minItems": 1,
      "maxItems": 10,
      "items": { // This part can already be defined using the built-in `Str`, `Integer`, etc classes
        "type": "string",
        "minLength": "1",
        "maxLength": "255"
      }
    }
  }
}

Valid value for the example:

{
  "tags": [
    "tag_1",
    "tag_2"
  ]
}

Invalid examples:

{
  "tags": "tag_1"
}
{
  "tags": [
    "tag_1",
    1000,
    false
  ]
}

Do you plan to support fields like this in the future? Or do you maybe already have a solution for this that I'm overlooking?

@tobyzerner
Copy link
Owner

tobyzerner commented Nov 7, 2023

Yeah would consider supporting some basic array schema/validation in an Array attribute subclass. Something like:

Field\Array::make('tags')
    ->minItems(1)
    ->maxItems(10)
    ->items(Field\Str::make()->minLength(1)->maxLength(255));

For now though, you have to implement validation manually, and while you can specify JSON schema it doesn't actually have any effect (will be used to generate OpenAPI definitions in the future).

Field\Attribute::make('tags')
    ->schema([
        'type' => 'array',
        'minItems' => 1,
        'maxItems' => 10,
        'items' => [
            'type' => 'string',
            'minLength' => 1,
            'maxLength' => 255,
        ],
    ])
    ->validate(function ($value, callable $fail, Context $context) {
        if (!is_array($value)) {
            $fail('must be an array');
            return;
        }

        // etc...
    });

Would also consider pulling in a JSON schema lib to perform validation based on any custom JSON schema you define by default, so you could just do:

Field\Attribute::make('tags')
    ->schema([
        'type' => 'array',
        'minItems' => 1,
        'maxItems' => 10,
        'items' => [
            'type' => 'string',
            'minLength' => 1,
            'maxLength' => 255,
        ],
    ]);

@tobyzerner tobyzerner added the enhancement New feature or request label Nov 7, 2023
@bertramakers
Copy link
Contributor Author

Yeah I was thinking of something like your first example. I even started implementing it myself but figured it would make more sense to have it in the library itself. If you want I can give it a shot to implement it?

Also about the OpenAPI definitions, do you have a timeline when that would be implemented? Or is there maybe already a WIP generator that I could test? I'm starting to get some demand for better documentation than a Postman collection, which is what I've been using now to share examples of how to use the API until the OpenAPI generator is ready. It's not urgent but just wondering :)

@tobyzerner
Copy link
Owner

Thanks for offering, would welcome a PR!

No timeline for OpenAPI definitions, but will keep that in mind and aim to find some time for it in the next month or two.

@bertramakers
Copy link
Contributor Author

bertramakers commented Nov 7, 2023

Alright, I need it for one of my upcoming tickets anyway so I'll try to implement it in a fork instead of in our app and make a PR if I can get it to work! Will probably require some additional testing/review on your end though, in case I overlook some things.

Edit to add: While trying to implement it before, I also thought of this part but struggled to implement it cleanly in our app:

->items(Field\Str::make()->minLength(1)->maxLength(255));

The issue I ran into is that make() and the constructor of the fields always require a name. Which makes sense of course, but in this case it feels a bit redundant. I thought of passing an empty name, or the same name as the Array field but neither feels really clean.

It's not really an issue to actually implement the Array field though, since it's more about the instantiation of the other field definition that is injected as the items field type. So I can already work on it while you think about what the best solution would be for this part.

@tobyzerner
Copy link
Owner

Thank you!

Yeah in that example I was thinking about the possibility of making the $name parameter in the field constructor optional (default to null). There may need to be an additional check somewhere that throws an error if you try to define a field without a name at the root level...

I'm not sure if there's a better solution, will keep thinking!

@bertramakers
Copy link
Contributor Author

Alternatively you could remove the name from the field allthogether and define it on the resource instead like this?

public function fields(): array
{
    return [
        'name' => Str::make()->...,
        'description' => Str::make()->...,
        'age' => Integer::make()->...,
    ];
}

Just a suggestion though, I'm not sure how feasible this is with the internals of the library.

@tobyzerner
Copy link
Owner

tobyzerner commented Nov 29, 2023

Thanks for the PR @bertramakers! At a first glance it looks good. I'm strongly considering moving the field name to the array keys as you have suggested above - while it will require a bit of work on the internals, it's not prohibitive, and I think the change makes semantic sense anyway (as field names are unique). I'll merge and then make this change soon.

@tobyzerner
Copy link
Owner

tobyzerner commented Nov 29, 2023

I realised that not only does it not make sense for array item schema to have a "name", but it also doesn't make sense for it to have a lot of the other functionality of the Field class – like visibility, serialize, writable, etc.

This prompted a refactor of "types" into their own concept instead of being coupled with "attributes". I've yet to update the docs for this change but as a quick example, instead of:

ArrayField::make('foo')
    ->minItems(10)
    ->items(Str::make('')->minLength(10))
    ->writable()

you now do:

Attribute::make('foo')
    ->type(
        Arr::make()
            ->minItems(10)
            ->items(Str::make()->minLength(10))
    )
    ->writable()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants