Improve Identification of Read-Only Field Status in Eloquent Fields

Description

The current implementation of read-only status for fields in the LaravelJsonApi\Eloquent\Fields\Concerns\IsReadOnly trait makes it challenging to determine which specific read-only method (readOnly(),readOnlyOnCreate(), or readOnlyOnUpdate()) has been called on a field. This lack of clarity can lead to difficulties in debugging and maintaining code, especially in complex schemas. This also makes it hard to create your own custom endpoints, that makes use of the attribute rules defined within the Schema.

Current Behavior

Currently, the read-only status is stored in a private property $readOnly as either a boolean or a Closure. The isReadOnly() method then evaluates this property to determine if a field is read-only. While functional, this approach doesn't provide an easy way to identify which specific read-only method was used.

/**
 * Whether the field is read-only.
 *
 * @var Closure|bool
 */
private $readOnly = false;

/**
 * Mark the field as read-only.
 *
 * @param Closure|bool $callback
 * @return $this
 */
public function readOnly($callback = true): self
{
    if (!is_bool($callback) && !$callback instanceof Closure) {
        throw new InvalidArgumentException('Expecting a boolean or closure.');
    }

    $this->readOnly = $callback;

    return $this;
}

/**
 * Mark the field as read only when the resource is being created.
 *
 * @return $this
 */
public function readOnlyOnCreate(): self
{
    $this->readOnly(static fn($request) => $request && $request->isMethod('POST'));

    return $this;
}

/**
 * Mark the field as read only when the resource is being updated.
 *
 * @return $this
 */
public function readOnlyOnUpdate(): self
{
    $this->readOnly(static fn($request) => $request && $request->isMethod('PATCH'));

    return $this;
}

Proposed Solution

To improve this, we could introduce a new property that explicitly tracks which read-only method was called. Here's a suggested implementation:

private $readOnlyType = null;

public function readOnly($callback = true): self
{
    // ... existing implementation ...
    $this->readOnlyType = 'always';
    return $this;
}

public function readOnlyOnCreate(): self
{
    // ... existing implementation ...
    $this->readOnlyType = 'onCreate';
    return $this;
}

public function readOnlyOnUpdate(): self
{
    // ... existing implementation ...
    $this->readOnlyType = 'onUpdate';
    return $this;
}

public function getReadOnlyType(): ?string
{
    return $this->readOnlyType;
}

This approach would allow developers to easily check which read-only method was called on a field:

$field = Number::make('target_value')->readOnlyOnUpdate();
echo $field->getReadOnlyType(); // Outputs: 'onUpdate'

Benefits

Improved debugging: Developers can quickly identify which read-only method was applied to a field.
Better introspection: Schemas become more self-documenting, as the read-only behavior of each field is explicitly tracked.
Easier maintenance: Changes to read-only behavior become more transparent and easier to manage.

Potential Impact

This change would be backwards-compatible, as it doesn't alter the existing behavior of the isReadOnly() method. It only adds new functionality to improve developer experience.

(Claude 3.5 Sonnet, has been used to help formulate this)

Hi. I'm not quite clear why this is needed? You say: "Developers can quickly identify which read-only method was applied to a field" but surely you can tell that by looking at the code in your schema? E.g. if I had a schema like this:

    public function fields(): array
    {
        return [
            ID::make(),
            BelongsTo::make('author')->type('users')->readOnly(),
            HasMany::make('comments')->canCount()->readOnly(),
            Str::make('content'),
            DateTime::make('createdAt')->sortable()->readOnlyOnUpdate(),
            SoftDelete::make('deletedAt')->sortable(),
            DateTime::make('publishedAt')->sortable(),
            Str::make('slug'),
            Str::make('synopsis'),
            BelongsToMany::make('tags'),
            Str::make('title')->sortable(),
            DateTime::make('updatedAt')->sortable()->readOnly(),
        ];
    }

I can see whether a field has use readOnly(), readOnlyOnCreate() or readOnlyOnUpdate(). So I can't see the reason why we need to record this on the field and add a getter method?

You also say: "This also makes it hard to create your own custom endpoints, that makes use of the attribute rules defined within the Schema." However, accessing individual fields from the schema is not mentioned anywhere in the docs, so this isn't supported behaviour.

Hi @lindyhopchris.

I would like to clarify that the primary goal of my proposal is to enable programmatic access to the read-only status of a field. While it's true that developers can see which method was called by looking at the schema code, there are scenarios where programmatic access would be beneficial. For instance:

When building tools or extensions that need to analyze or introspect schemas
For generating documentation or API specifications automatically

I understand that accessing individual fields from the schema isn't currently supported behavior. However, I believe there's value in considering such an extension. In our case, we needed to implement custom endpoints while staying as true to the schema as possible. In fact we already managed to achieve this as shown below, however the way of doing it is less than optimal.

Validating and serializing:

$validatedAttributes = [];
foreach ($schema->fields() as $field) {
  if ($field instanceof \LaravelJsonApi\Eloquent\Fields\Attribute) {
    $key = $field->name();
    $value = $field->serialize($model);
    $validatedAttributes[$key] = $value;
  } elseif ($field instanceof \LaravelJsonApi\Eloquent\Fields\ID) {
    $key = 'id';
    $value = $model->id;
    $validatedAttributes[$key] = $value;
  } elseif ($field instanceof \LaravelJsonApi\Eloquent\Fields\Relations\Relation) {
    continue; # Skip relations for now. Might be implemented in the future.
  } else {
    throw new InvalidArgumentException('Field must be an instance of Attribute or Relation');
  }
}

return $validatedAttributes;

Checking ´read-only´ status:

// Check if we are updating or creating
$isCreating = $model->exists();

// Get the field
$field = $schema->field($attributeName);

// Since $field->isReadOnly($request) calls $request->isMethod('METHOD') to see if we are creating or updating
// we are creating "fake" request objects to provide it.
$patchMockRequest = Request::create(uri: 'DOES NOT MATTER', method: 'PATCH');
$postMockRequest = Request::create(uri: 'DOES NOT MATTER', method: 'POST');

$isReadOnlyUnconditional = $field->isReadOnly(null);
$isReadOnlyWhenCreating = $isCreating && $field->isReadOnly($postMockRequest);
$isReadOnlyWhenUpdating = !$isCreating && $field->isReadOnly($patchMockRequest);

While I recognize that this isn't currently supported behavior, I don't see a fundamental reason why the package couldn't be expanded to support such use cases. Opening up the package to allow for more introspection could potentially broaden its applicability and usefulness in more complex scenarios.

I appreciate that this might not align with the current vision for the package. However, I believe that providing more programmatic access to schema information could open up new possibilities for users of the package, especially those dealing with more complex or custom implementations.

I'm all in favour for opening the package up for documentation - it's a longer term goal to head in that direction. There's some quite substantial changes coming to schemas and fields to move validation into those fields, and that should help with this.

The problem with your proposed approach is you are assuming that people use readOnlyOnCreate() and readOnlyOnUpdate(). These are just convenience methods. The actual usage could look like this (from the docs):

Str::make('name')->readOnly(
    static fn($request) => !$request->user()?->isAdmin()
)

What then would your new getReadOnlyType() return in this instance? This example isn't an edge case, anything custom like this could be being used in a lot of places.

So while I agree with the intent of your proposal, I don't think the solution works. The longer term aim is to support documentation via OpenAPI. Unfortunately I'm not an OpenAPI expert. I would be interested in knowing from someone who is, how OpenAPI would document the read-only status of a field when that read-only status may be dependent on different aspects of the request e.g. if it's create, update, admins only, etc. That might lead us to a solution.

Right, I see where you are coming from. I see that solely being able to differentiate between readOnlyOnCreate() and readOnlyOnUpdate() would be too specific and short sighted. Having a solution that would allow to make it possible to identify custom "checks" with lack of better words would be preferred.

What if we did something like this:

/**
 * Whether the field is read-only.
 *
 * @var Closure|bool
 */
private $readOnly = false;

/**
 * Identifies the type of readOnly rule
 *
 * @var string|null
 */
private $readOnlyIdentifier = null;

/**
 * Mark the field as read-only.
 *
 * @param Closure|bool $callback
 * @param string|null $identifier
 * @return $this
 */
public function readOnly($callback = true, $identifier = null): self
{
    if (!is_bool($callback) && !$callback instanceof Closure) {
        throw new InvalidArgumentException('Expecting a boolean or closure.');
    }

    $this->readOnly = $callback;
    $this->readOnlyIdentifier = $identifier;

    return $this;
}

/**
 * Get the identifier for the read-only rule.
 *
 * @return string|null
 */
public function getReadOnlyIdentifier(): ?string
{
    return $this->readOnlyIdentifier;
}

We could then use it like this:

$field = Str::make('name')->readOnly(static fn($request) => !$request->user()?->isAdmin(), 'adminOnly');

echo $field->getReadOnlyIdentifier(); // Outputs: 'adminOnly'

This approach would allow us to keep the existing functionality while adding the ability to identify custom read-only rules. This is would also be backwards compatible, while being a simple solution.

To maintain compatibility with the existing helper methods, we could update them like this:

/**
 * Mark the field as read only when the resource is being created.
 *
 * @return $this
 */
public function readOnlyOnCreate(): self
{
    return $this->readOnly(
        static fn($request) => $request && $request->isMethod('POST'),
        'onCreate'
    );
}

/**
 * Mark the field as read only when the resource is being updated.
 *
 * @return $this
 */
public function readOnlyOnUpdate(): self
{
    return $this->readOnly(
        static fn($request) => $request && $request->isMethod('PATCH'),
        'onUpdate'
    );
}

This solution addresses several key points:

Flexibility: It allows for custom identifiers for any read-only rule, not just predefined ones.
Backwards Compatibility: Existing code will continue to work without modification.
Simplicity: It's a straightforward addition to the existing API, requiring minimal changes.
Extensibility: This approach could potentially be applied to other field configurations in the future if needed.

This solution strikes a balance between providing more information for documentation and introspection purposes while maintaining the flexibility and simplicity of the current implementation. What are your thoughts on this approach? Do you see any issues or potential for improvement?

laravel-json-api / laravel