[Ruby] Added validations resulting in a breaking changes

[tomdracz]

We're using API generator for Ruby to generate OpenAPI clients from the spec file.

We've just generated new client using version 7.12.0 but this resulted in numerous breaking changes.

Those were caused by additions in #20672 that weren't marked as breaking.

Is raising errors on initialization a sensible approach? This feels slightly off. Consider a example API entity of user with required name attribute.

Calling generated model initialization method without arguments like

MyApi::User.new

Was working previously but will now raise an error as the name would be assigned to nil and the changes in setter method name= will mean it immediately fails.

In our apps this has manifested mostly in two places:

• specs constructing API gem models through FactoryBot no longer work. Workaround is to make sure to use FactoryBot initialize_with declaration, otherwise I think default behaviour is to initialize empty model and then mass assign attributes later
• Pact tests that were previously using partial responses for verification of what's required, now need every required field present, otherwise they will raise errors on initialization too.

Keen for any potential workarounds and other thoughts. Again the change was not noted as breaking but has got considerable knock on effect.

[wing328]

@tomdracz thanks for reporting the issue. I merged it as bug fixes. Sorry that it breaks your use cases.

@yujideveloper can you please review the feedback from @tomdracz when you've time? Thanks.

[wing328]

Was working previously but will now raise an error as the name would be assigned to nil and the changes in setter method name= will mean it immediately fails.

does it mean you're assuming primitive type are nullable by default in your use cases?

if that's the case, then there's a normalizer rule for that: SET_PRIMITIVE_TYPES_TO_NULLABLE

ref: https://github.com/openapitools/openapi-generator/blob/master/docs/customization.md#openapi-normalizer

[tomdracz]

does it mean you're assuming primitive type are nullable by default in your use cases?

Not quite. It's around any attribute. Consider schema entity like:

Rfq:
  type: "object"
  properties:
    reference:
      type: "string"
      description: "A unique identifier"
      example: "NB324343"
    customer:
      $ref: "#/components/schemas/Customer"
    created_at:
      type: "string"
      description: "The time at which the RFQ was created."
      example: "2020-10-26T12:35:11Z"
  required:
    - reference
    - customer
    - created_at

The generated model will create setter methods for all three fields and set them by default as nil, making them always required in initialisation.

So FactoryBot configuration like:

  factory :rfq, class: "Api::Rfq" do
    created_at { "2023-12-20T09:59:10+01:00" }
    reference { "ABC100001" }
  end

Will immediately raise when calling something like FactoryBot.build(:rfq).

It will blow up also even when customer is present like:

  factory :rfq, class: "Api::Rfq" do
    created_at { "2023-12-20T09:59:10+01:00" }
    reference { "ABC100001" }
    customer { build(:customer) }
  end

Unless initialize_with { new(attributes) } is added as I think FactoryBot will start with an object with all attributes as nils.

For bigger objects, this creates lots of noise and necessitates defining all-encompassing factories, even when they are not required.

Similarly, with Pact specs, when mocking some expectations it would not be unusual to only define partial objects to test for what's expected, but again, with the changes it necessitates creation of complete fixtures.

Since this is squarely in the realms of testing (as deployed implementation works), it might be more of a "me" problem than "you" problem, but it did create quite a lot of havoc and we've rolled back to using v7.11.0 for now as we see no other workarounds available

[yujideveloper]

Is raising errors on initialization a sensible approach?

I don't like this approach that raises ArgumentError in the initializer.
While it enforces strict validation, it can lead to disruptions in scenarios where flexibility is expected.

However, this approach wasn't introduced in #20672; it was first discussed in #15895 and implemented in #16114.
The foundation for raising ArgumentError in initializers was implemented in #16114, where properties with validations other than "required" and "not null" were already validated at initialization, causing errors if conditions weren't met.

#20672 is a follow-up to #16114 and implements missing validation patterns for required and non-nullable properties.
Its intent is to resolve inconsistencies in the validation behavior rather than to introduce new patterns.
In this regard, I believe #20672 is a fix for a consistency issue.

P.S.
I feel it might be better to revert both #16114 and #20672 and reconsider the deprecation of #valid? and #list_invalid_properties.

[balvig]

For what it's worth, I've run into this as well! 🙇

While it's fantastic that the generator creates validation logic from the schema, it was surprising that it happens when initializing a new object, rather than "on demand".

In my case, I'm using models as form objects in Rails.
Here is a simplified example:

class AccountsController < ApplicationController
  def new
    @account_request = MyApi::CreateAccountRequest.new
  end

  def create
    @account_request = MyApi::CreateAccountRequest.new(account_params)
    MyApi::AccountsApi.new.create(@account_request)

    redirect_to root_path
  rescue MyApi::ApiError
    render :new, status: :unprocessable_entity
  end

  private

  def account_params
    params.require(:account_request).permit(:email, :name, :password).to_h
  end
end

<%= form_with model: @account_request, url: accounts_path, method: :post, scope: "account_request" do |f| %>
  <%= f.label :name %>
  <%= f.text_field :name %>

  <%= f.label :email %>
  <%= f.text_field :email %>

  <%= f.label :password %>
  <%= f.password_field :password %>

  <%= f.submit %>
<% end %>

This throws an error in the new action, since I can't instantiate a CreateAccountRequest with a blank email, name, or password:

ArgumentError: invalid value for "name", the character length must be great than or equal to 1.

It sounds like being able to manually call valid? to trigger client-side validations only when needed would be ideal. 🤔

[wing328]

what about adding an option to skip validation in the constructor?

[balvig]

what about adding an option to skip validation in the constructor?

@wing328 thanks for that idea!

Although that might solve the problem to some extent, I worry it could turn into a gotcha since the accessors would behave differently, eg:

request = CreateAccountRequest.new(email: "") # works
request.email = "" # ArgumentError

I think I agree with @yujideveloper that it might be worth reconsidering the previous valid? approach to trigger validation on demand, rather than during instantiation. 🤔

I'd be happy to take a stab at it as well if we're satisfied with that direction!