Revert "Fix address model validation" #244
Conversation
This reverts commit 2f38d82.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mananshah78424, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request aims to revert a previous change related to address model validation by consolidating the UsAddress and IntlAddress models back into a single, unified Address model within the lob-ruby SDK. This unification streamlines address handling, updates relevant documentation and tests, and includes general improvements to pagination logic in integration tests. The changes also reflect a significant update to the SDK's OpenAPI client, leading to a major version increment.
Highlights
- Address Model Consolidation: The previous separate
UsAddressandIntlAddressmodels have been consolidated into a singleLob::Addressclass. This simplifies the core address data structure and its handling within the SDK. - Updated Address Validation Logic: The unified
Lob::Addressmodel now includes direct validation rules for address components, specifically enforcing US-centric formats foraddress_state(2-letter code) andaddress_zip(ZIP or ZIP+4 format). - API Client Regeneration and Version Bump: New files related to the OpenAPI client generation have been added, indicating a significant update to the SDK's underlying API client structure. This change is accompanied by a major version bump to
7.0.0, suggesting breaking changes. - Integration Test Pagination Fixes: Integration tests for
Check,Letter, andTemplateAPIs have been corrected to properly handle pagination, specifically by usingresponse.next_urlwhen fetching items with thebeforeparameter.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request reverts a previous change related to address model validation. While the revert itself seems intended, it has introduced several issues:
- Critical: The
Addressmodel now has incorrect validation foraddress_stateandaddress_zipfor international addresses, which is a major regression. - Critical: Pagination tests in several integration test files are broken due to incorrect URL parsing.
- High: A number of new, unreferenced files under
lib/openapi_client/seem to have been added by mistake and should be removed. - Medium: A leftover debug statement was found in one of the tests.
These issues, especially the validation and test breakages, should be addressed before merging.
| expect(response.data[0].id).not_to eq(first_chk_id) | ||
|
|
||
| uri = URI.parse(response.previous_url) | ||
| uri = URI.parse(response.next_url) |
There was a problem hiding this comment.
This line appears to contain a bug. You are parsing response.next_url but then attempting to extract a before token from it to test before pagination. The next_url contains an after token, not a before token. This will likely cause params["before"] to be nil and raise an error on the following line. You should probably be parsing response.previous_url here to get the before token.
uri = URI.parse(response.previous_url)| expect(response.data[0].id).not_to eq(first_ltr_id) | ||
|
|
||
| uri = URI.parse(response.previous_url) | ||
| uri = URI.parse(response.next_url) |
There was a problem hiding this comment.
This line appears to contain a bug. You are parsing response.next_url but then attempting to extract a before token from it to test before pagination. The next_url contains an after token, not a before token. This will likely cause params["before"] to be nil and raise an error on the following line. You should probably be parsing response.previous_url here to get the before token.
uri = URI.parse(response.previous_url)| expect(response.data[0].id).not_to eq(first_tmpl_id) | ||
|
|
||
| uri = URI.parse(response.previous_url) | ||
| uri = URI.parse(response.next_url) |
There was a problem hiding this comment.
This line appears to contain a bug. You are parsing response.next_url but then attempting to extract a before token from it to test before pagination. The next_url contains an after token, not a before token. This will likely cause params["before"] to be nil and raise an error on the following line. You should probably be parsing response.previous_url here to get the before token.
uri = URI.parse(response.previous_url)| pattern = Regexp.new(/^[a-zA-Z]{2}$/) | ||
| if !address_state.nil? && address_state !~ pattern | ||
| fail ArgumentError, "invalid value for \"address_state\", must conform to the pattern #{pattern}." | ||
| end | ||
|
|
||
| @address_state = address_state | ||
| end | ||
|
|
||
| # Custom attribute writer method with validation | ||
| # @param [Object] address_zip Value to be assigned | ||
| def address_zip=(address_zip) | ||
| pattern = Regexp.new(/^\d{5}(-\d{4})?$/) | ||
| if !address_zip.nil? && address_zip !~ pattern | ||
| fail ArgumentError, "invalid value for \"address_zip\", must conform to the pattern #{pattern}." |
There was a problem hiding this comment.
These validation patterns for address_state and address_zip are specific to US addresses. This will cause validation to fail for international addresses, which is a significant regression.
The previous implementation using oneOf with UsAddress and IntlAddress likely handled this correctly by providing different validation rules for each. Reverting to a single Address model has reintroduced this issue. This needs to be addressed to restore support for international addresses.
| =begin | ||
| #Lob | ||
| #The Lob API is organized around REST. Our API is designed to have predictable, resource-oriented URLs and uses HTTP response codes to indicate any API errors. <p> Looking for our [previous documentation](https://lob.github.io/legacy-docs/)? | ||
| The version of the OpenAPI document: 1.3.0 | ||
| Contact: lob-openapi@lob.com | ||
| Generated by: https://openapi-generator.tech | ||
| OpenAPI Generator version: 5.2.1 | ||
| =end | ||
|
|
||
| # Common files | ||
| require 'openapi_client/api_client' | ||
| require 'openapi_client/api_error' | ||
| require 'openapi_client/version' | ||
| require 'openapi_client/configuration' | ||
|
|
||
| # Models | ||
| require 'lob/models/address' | ||
| require 'lob/models/address_deletion' | ||
| require 'lob/models/address_domestic' | ||
| require 'lob/models/address_domestic_expanded' | ||
| require 'lob/models/address_editable' | ||
| require 'lob/models/address_list' | ||
| require 'lob/models/bank_account' | ||
| require 'lob/models/bank_account_deletion' | ||
| require 'lob/models/bank_account_list' | ||
| require 'lob/models/bank_account_verify' | ||
| require 'lob/models/bank_account_writable' | ||
| require 'lob/models/bank_type_enum' | ||
| require 'lob/models/billing_group' | ||
| require 'lob/models/billing_group_editable' | ||
| require 'lob/models/billing_group_list' | ||
| require 'lob/models/buckslip' | ||
| require 'lob/models/buckslip_deletion' | ||
| require 'lob/models/buckslip_editable' | ||
| require 'lob/models/buckslip_order' | ||
| require 'lob/models/buckslip_order_editable' | ||
| require 'lob/models/buckslip_orders_list' | ||
| require 'lob/models/buckslip_updatable' | ||
| require 'lob/models/buckslips_list' | ||
| require 'lob/models/bulk_error' | ||
| require 'lob/models/bulk_error_properties' | ||
| require 'lob/models/campaign' | ||
| require 'lob/models/campaign_creative' | ||
| require 'lob/models/campaign_deletion' | ||
| require 'lob/models/campaign_updatable' | ||
| require 'lob/models/campaign_writable' | ||
| require 'lob/models/campaigns_list' | ||
| require 'lob/models/card' | ||
| require 'lob/models/card_deletion' | ||
| require 'lob/models/card_editable' | ||
| require 'lob/models/card_list' | ||
| require 'lob/models/card_order' | ||
| require 'lob/models/card_order_editable' | ||
| require 'lob/models/card_order_list' | ||
| require 'lob/models/card_updatable' | ||
| require 'lob/models/check' | ||
| require 'lob/models/check_deletion' | ||
| require 'lob/models/check_editable' | ||
| require 'lob/models/check_list' | ||
| require 'lob/models/chk_use_type' | ||
| require 'lob/models/cmp_schedule_type' | ||
| require 'lob/models/cmp_use_type' | ||
| require 'lob/models/country_extended' | ||
| require 'lob/models/country_extended_expanded' | ||
| require 'lob/models/creative_patch' | ||
| require 'lob/models/creative_response' | ||
| require 'lob/models/creative_writable' | ||
| require 'lob/models/custom_envelope_returned' | ||
| require 'lob/models/deliverability_analysis' | ||
| require 'lob/models/dpv_footnote' | ||
| require 'lob/models/engine_html' | ||
| require 'lob/models/event_type' | ||
| require 'lob/models/events' | ||
| require 'lob/models/export' | ||
| require 'lob/models/export_model' | ||
| require 'lob/models/geocode_addresses' | ||
| require 'lob/models/geocode_components' | ||
| require 'lob/models/http_validation_error' | ||
| require 'lob/models/identity_validation' | ||
| require 'lob/models/inline_object' | ||
| require 'lob/models/intl_autocompletions' | ||
| require 'lob/models/intl_autocompletions_writable' | ||
| require 'lob/models/intl_components' | ||
| require 'lob/models/intl_suggestions' | ||
| require 'lob/models/intl_verification' | ||
| require 'lob/models/intl_verification_or_error' | ||
| require 'lob/models/intl_verification_writable' | ||
| require 'lob/models/intl_verifications' | ||
| require 'lob/models/intl_verifications_payload' | ||
| require 'lob/models/letter' | ||
| require 'lob/models/letter_custom_envelope' | ||
| require 'lob/models/letter_deletion' | ||
| require 'lob/models/letter_details_returned' | ||
| require 'lob/models/letter_details_writable' | ||
| require 'lob/models/letter_editable' | ||
| require 'lob/models/letter_list' | ||
| require 'lob/models/lob_confidence_score' | ||
| require 'lob/models/lob_error' | ||
| require 'lob/models/location' | ||
| require 'lob/models/location_analysis' | ||
| require 'lob/models/ltr_use_type' | ||
| require 'lob/models/mail_type' | ||
| require 'lob/models/multi_line_address' | ||
| require 'lob/models/multiple_components' | ||
| require 'lob/models/multiple_components_intl' | ||
| require 'lob/models/multiple_components_list' | ||
| require 'lob/models/optional_address_column_mapping' | ||
| require 'lob/models/placeholder_model' | ||
| require 'lob/models/postcard' | ||
| require 'lob/models/postcard_deletion' | ||
| require 'lob/models/postcard_details_returned' | ||
| require 'lob/models/postcard_details_writable' | ||
| require 'lob/models/postcard_editable' | ||
| require 'lob/models/postcard_list' | ||
| require 'lob/models/postcard_size' | ||
| require 'lob/models/psc_use_type' | ||
| require 'lob/models/qr_code' | ||
| require 'lob/models/required_address_column_mapping' | ||
| require 'lob/models/return_envelope' | ||
| require 'lob/models/reverse_geocode' | ||
| require 'lob/models/self_mailer' | ||
| require 'lob/models/self_mailer_deletion' | ||
| require 'lob/models/self_mailer_editable' | ||
| require 'lob/models/self_mailer_list' | ||
| require 'lob/models/self_mailer_size' | ||
| require 'lob/models/sfm_use_type' | ||
| require 'lob/models/sort_by' | ||
| require 'lob/models/sort_by1' | ||
| require 'lob/models/sort_by2' | ||
| require 'lob/models/sort_by3' | ||
| require 'lob/models/sort_by_date_modified' | ||
| require 'lob/models/suggestions' | ||
| require 'lob/models/template' | ||
| require 'lob/models/template_deletion' | ||
| require 'lob/models/template_list' | ||
| require 'lob/models/template_update' | ||
| require 'lob/models/template_version' | ||
| require 'lob/models/template_version_deletion' | ||
| require 'lob/models/template_version_list' | ||
| require 'lob/models/template_version_updatable' | ||
| require 'lob/models/template_version_writable' | ||
| require 'lob/models/template_writable' | ||
| require 'lob/models/thumbnail' | ||
| require 'lob/models/tracking_event_certified' | ||
| require 'lob/models/tracking_event_details' | ||
| require 'lob/models/tracking_event_normal' | ||
| require 'lob/models/upload' | ||
| require 'lob/models/upload_create_export' | ||
| require 'lob/models/upload_file' | ||
| require 'lob/models/upload_state' | ||
| require 'lob/models/upload_updatable' | ||
| require 'lob/models/upload_writable' | ||
| require 'lob/models/uploads_metadata' | ||
| require 'lob/models/us_autocompletions' | ||
| require 'lob/models/us_autocompletions_writable' | ||
| require 'lob/models/us_components' | ||
| require 'lob/models/us_verification' | ||
| require 'lob/models/us_verification_or_error' | ||
| require 'lob/models/us_verifications' | ||
| require 'lob/models/us_verifications_writable' | ||
| require 'lob/models/validation_error' | ||
| require 'lob/models/zip' | ||
| require 'lob/models/zip_code_type' | ||
| require 'lob/models/zip_editable' | ||
| require 'lob/models/zip_lookup_city' | ||
|
|
||
| # APIs | ||
| require 'lob/api/addresses_api' | ||
| require 'lob/api/bank_accounts_api' | ||
| require 'lob/api/billing_groups_api' | ||
| require 'lob/api/buckslip_orders_api' | ||
| require 'lob/api/buckslips_api' | ||
| require 'lob/api/campaigns_api' | ||
| require 'lob/api/card_orders_api' | ||
| require 'lob/api/cards_api' | ||
| require 'lob/api/checks_api' | ||
| require 'lob/api/creatives_api' | ||
| require 'lob/api/default_api' | ||
| require 'lob/api/identity_validation_api' | ||
| require 'lob/api/intl_autocompletions_api' | ||
| require 'lob/api/intl_verifications_api' | ||
| require 'lob/api/letters_api' | ||
| require 'lob/api/postcards_api' | ||
| require 'lob/api/reverse_geocode_lookups_api' | ||
| require 'lob/api/self_mailers_api' | ||
| require 'lob/api/template_versions_api' | ||
| require 'lob/api/templates_api' | ||
| require 'lob/api/uploads_api' | ||
| require 'lob/api/us_autocompletions_api' | ||
| require 'lob/api/us_verifications_api' | ||
| require 'lob/api/zip_lookups_api' | ||
|
|
||
| module OpenapiClient | ||
| class << self | ||
| # Customize default settings for the SDK using block. | ||
| # OpenapiClient.configure do |config| | ||
| # config.username = "xxx" | ||
| # config.password = "xxx" | ||
| # end | ||
| # If no block given, return the default Configuration object. | ||
| def configure | ||
| if block_given? | ||
| yield(Configuration.default) | ||
| else | ||
| Configuration.default | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
This file and the other files under lib/openapi_client/ seem to have been added by mistake as part of this revert. They introduce a new OpenapiClient module that is not referenced anywhere else in the codebase, and the existing code uses the Lob module. These files also define classes like ApiClient that could conflict with existing classes in the Lob module. These files should probably be removed to avoid confusion and potential conflicts.
|
|
||
| it "lists addresses with after param" do | ||
| response = @addressApi.list(limit: 2) | ||
| response = @addressApi.list() |
There was a problem hiding this comment.
This test for pagination is less reliable without a limit parameter. To ensure a paginated response and the presence of a next_page_token, it's better to explicitly set a limit, as was done in the code being reverted.
response = @addressApi.list(limit: 2)|
|
||
| it "lists addresses with before param" do | ||
| response = @addressApi.list(limit: 2) | ||
| response = @addressApi.list() |
There was a problem hiding this comment.
Similar to the test for the after parameter, this test for the before parameter is less reliable without an explicit limit. Using a limit ensures that a paginated response is returned, which is necessary for robustly testing pagination.
response = @addressApi.list(limit: 2)|
|
||
| it "Autocompletes US address (with test key)" do | ||
| autocompletedAddr = @usAutocompletionApi.autocomplete(@validAddress) | ||
| puts autocompletedAddr.suggestions[0].primary_line |
There was a problem hiding this comment.
This puts statement appears to be a leftover debugging statement. It should be removed to keep the test output clean.
| | **address_state** | **String** | 2 letter state short-name code | [optional] | | ||
| | **address_zip** | **String** | Must follow the ZIP format of `12345` or ZIP+4 format of `12345-1234`. | [optional] | |
There was a problem hiding this comment.
The descriptions for address_state and address_zip have been updated to reflect US-specific formats. This is misleading for users creating international addresses. The documentation should clarify that these formats are for US addresses and provide guidance for international address formats, or the validation in the model should be updated to handle both.
Reverts #243