Skip to content

Fix monkey patching#3016

Merged
MonkeyCanCode merged 15 commits intoapache:mainfrom
MonkeyCanCode:fix_monkey_patching
Nov 12, 2025
Merged

Fix monkey patching#3016
MonkeyCanCode merged 15 commits intoapache:mainfrom
MonkeyCanCode:fix_monkey_patching

Conversation

@MonkeyCanCode
Copy link
Contributor

This PR cleans up how OpenAPI-generated model imports for policies are handled. Previously, we used a runtime patch _patch_generated_models() in polaris_cli.py to work around missing policies imports. That logic has now been moved to the client generation process in generate_clients.py.

A new helper function, fix_catalog_models_init(), was added to regenerate the apache_polaris.sdk.catalog.models.__init__.py file so it correctly imports all model classes. This makes the generated client more reliable and removes the need for any runtime patching (or more monkey patching later as we have more classes added which is not part of Iceberg catalog spec).

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

@MonkeyCanCode
Copy link
Contributor Author

Here is the full diff between the new code and old code:

➜  polaris git:(fix_monkey_patching) diff -r client/python/apache_polaris ~/Desktop/current/polaris/client/python/apache_polaris
diff --color -r client/python/apache_polaris/cli/polaris_cli.py /Users/yong/Desktop/old/polaris/client/python/apache_polaris/cli/polaris_cli.py
49a50,90
>     def _patch_generated_models() -> None:
>         """
>         The OpenAPI generator creates an `api_client` that dynamically looks up
>         model classes from the `apache_polaris.sdk.catalog.models` module using `getattr()`.
>         For example, when a response for a `create_policy` call is received, the
>         deserializer tries to find the `LoadPolicyResponse` class by looking for
>         `apache_polaris.sdk.catalog.models.LoadPolicyResponse`.
>
>         However, the generator fails to add the necessary `import` statements
>         to the `apache_polaris/sdk/catalog/models/__init__.py` file. This means that even
>         though the model files exist (e.g., `load_policy_response.py`), the classes
>         are not part of the `apache_polaris.sdk.catalog.models` namespace.
>
>         This method works around the bug in the generated code without modifying
>         the source files. It runs once per CLI execution, before any commands, and
>         manually injects the missing response-side model classes into the
>         `apache_polaris.sdk.catalog.models` namespace, allowing the deserializer to find them.
>         """
>         import apache_polaris.sdk.catalog.models
>         from apache_polaris.sdk.catalog.models.applicable_policy import ApplicablePolicy
>         from apache_polaris.sdk.catalog.models.get_applicable_policies_response import GetApplicablePoliciesResponse
>         from apache_polaris.sdk.catalog.models.list_policies_response import ListPoliciesResponse
>         from apache_polaris.sdk.catalog.models.load_policy_response import LoadPolicyResponse
>         from apache_polaris.sdk.catalog.models.policy import Policy
>         from apache_polaris.sdk.catalog.models.policy_attachment_target import PolicyAttachmentTarget
>         from apache_polaris.sdk.catalog.models.policy_identifier import PolicyIdentifier
>
>         models_to_patch = {
>             "ApplicablePolicy": ApplicablePolicy,
>             "GetApplicablePoliciesResponse": GetApplicablePoliciesResponse,
>             "ListPoliciesResponse": ListPoliciesResponse,
>             "LoadPolicyResponse": LoadPolicyResponse,
>             "Policy": Policy,
>             "PolicyAttachmentTarget": PolicyAttachmentTarget,
>             "PolicyIdentifier": PolicyIdentifier,
>         }
>
>         for name, model_class in models_to_patch.items():
>             setattr(apache_polaris.sdk.catalog.models, name, model_class)
>
>     @staticmethod
50a92
>         PolarisCli._patch_generated_models()
diff --color -r client/python/apache_polaris/sdk/catalog/models/__init__.py /Users/yong/Desktop/old/polaris/client/python/apache_polaris/sdk/catalog/models/__init__.py
19a20,34
> # coding: utf-8
>
> # flake8: noqa
> """
>     Apache Iceberg REST Catalog API
>
>     Defines the specification for the first version of the REST Catalog API. Implementations should ideally support both Iceberg table specs v1 and v2, with priority given to v2.
>
>     The version of the OpenAPI document: 0.0.1
>     Generated by OpenAPI Generator (https://openapi-generator.tech)
>
>     Do not edit the class manually.
> """  # noqa: E501
>
> # import models into model package
27d41
< from apache_polaris.sdk.catalog.models.applicable_policy import ApplicablePolicy
39d52
< from apache_polaris.sdk.catalog.models.attach_policy_request import AttachPolicyRequest
53d65
< from apache_polaris.sdk.catalog.models.create_generic_table_request import CreateGenericTableRequest
56d67
< from apache_polaris.sdk.catalog.models.create_policy_request import CreatePolicyRequest
61d71
< from apache_polaris.sdk.catalog.models.detach_policy_request import DetachPolicyRequest
74,75d83
< from apache_polaris.sdk.catalog.models.generic_table import GenericTable
< from apache_polaris.sdk.catalog.models.get_applicable_policies_response import GetApplicablePoliciesResponse
78,79d85
< from apache_polaris.sdk.catalog.models.iceberg_error_response1 import IcebergErrorResponse1
< from apache_polaris.sdk.catalog.models.list_generic_tables_response import ListGenericTablesResponse
81d86
< from apache_polaris.sdk.catalog.models.list_policies_response import ListPoliciesResponse
86,87d90
< from apache_polaris.sdk.catalog.models.load_generic_table_response import LoadGenericTableResponse
< from apache_polaris.sdk.catalog.models.load_policy_response import LoadPolicyResponse
95,96d97
< from apache_polaris.sdk.catalog.models.notification_request import NotificationRequest
< from apache_polaris.sdk.catalog.models.notification_type import NotificationType
106,108d106
< from apache_polaris.sdk.catalog.models.policy import Policy
< from apache_polaris.sdk.catalog.models.policy_attachment_target import PolicyAttachmentTarget
< from apache_polaris.sdk.catalog.models.policy_identifier import PolicyIdentifier
121a120
> from apache_polaris.sdk.catalog.models.sql_view_representation import SQLViewRepresentation
141d139
< from apache_polaris.sdk.catalog.models.sql_view_representation import SQLViewRepresentation
150d147
< from apache_polaris.sdk.catalog.models.table_update_notification import TableUpdateNotification
160d156
< from apache_polaris.sdk.catalog.models.update_policy_request import UpdatePolicyRequest
165,166d160
< from apache_polaris.sdk.catalog.models.view_representation import ViewRepresentation
< from apache_polaris.sdk.catalog.models.view_requirement import ViewRequirement
168c162,163
< from apache_polaris.sdk.catalog.models.view_version import ViewVersion
\ No newline at end of file
---
> from apache_polaris.sdk.catalog.models.view_version import ViewVersion
>

snazy
snazy previously approved these changes Nov 11, 2025
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever approach to parse Python-source-AST in Python ;)

Just one nit and a potential improvement.
But no need to add it to this PR, so +1

for model_file in sorted(model_files):
module_name = model_file.stem
with open(model_file, "r") as f:
tree = ast.parse(f.read())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever :)

break
if class_name:
imports.append(
f"from apache_polaris.sdk.catalog.models.{module_name} import {class_name}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it makes sense to check for multiple classes to import from a single module (like from x.y.z import A, B, C)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is possible to do. But I thought OpenAPI generator always produces model files with only one public class (I may be wrong here). If there is needed, maybe we can add it as an enhancement PR instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, was just a comment. Nothing to do here :)

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Nov 11, 2025
@snazy
Copy link
Member

snazy commented Nov 11, 2025

Manually verified that the generated __init__.py contains all imports that the previous approach generated, just that this approach doesn't miss some types (i.e. this approach generates the complete set of import statements).

renovate-bot and others added 12 commits November 11, 2025 22:13
A project for JMH based benchmarks against NoSQL persistence.
Introduces handling for realms including realm-state management/transition.

The `RealmStore` implementation for NoSQL depends on CDI components, coming in a follo-up PR.
* rename AccessConfig for clarity

* rename getStorageAccessConfig() and add javadoc
* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles
- add `StorageCredentialsVendor` as request-scoped wrapper around `PolarisCredentialVendor`
- make `FileIOFactory` request-scoped
- make `TaskFileIOSupplier` request-scoped
This is to fix javadoc error: `No public or protected classes found to document`
@MonkeyCanCode MonkeyCanCode requested a review from snazy November 12, 2025 04:15
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement!

@MonkeyCanCode MonkeyCanCode merged commit 6a335a1 into apache:main Nov 12, 2025
15 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Nov 12, 2025
snazy added a commit to snazy/polaris that referenced this pull request Feb 11, 2026
* Add loadEntities batch call and rename listFullEntities (apache#2508)

* Add loadEntities batch call and rename listFullEntities

* Changed batch call to implement loadResolvedEntities instead

* Add loadResolvedEntities by id and entity cache support

* Add additional test for loadResolvedEntities by id

* Added additional test and updated comments in EntityCache interface

* Add additional constructor to ResolvedEntitiesResult

* Fixed unused method reference

* Removed loadResolvedEntities method with lookup record param

* Pulled out toResolvedPolarisEntity method per PR comment

* Core: made the ARN role regex more generic (apache#3005)

* fix(docs): Generify S3 index page (apache#2997)

* Remove the mention of "cloud" since not all possible storage options are provided in "cloud".

* Avoid listing specific child pages in the doc test. Rely on Hugo-general index (on the left-hand pane).

---------

Co-authored-by: Alexandre Dutra <adutra@apache.org>

* fix(deps): update dependency io.prometheus:prometheus-metrics-exporter-servlet-jakarta to v1.4.3 (apache#3009)

* fix(deps): update dependency com.google.cloud:google-cloud-storage-bom to v2.60.0 (apache#3011)

* fix(deps): update dependency io.opentelemetry:opentelemetry-bom to v1.56.0 (apache#3012)

* fix(deps): update dependency com.adobe.testing:s3mock-testcontainers to v4.10.0 (apache#3010)

* fix(deps): update dependency org.agrona:agrona to v2.3.2 (apache#3014)

* fix(deps): update quarkus platform and group to v3.29.2 (apache#3013)

* chore(deps): update dependency pre-commit to v4.4.0 (apache#3015)

* fix(deps): update dependency software.amazon.awssdk:bom to v2.38.2 (apache#3019)

* Add test for TracingFilter (apache#2847)

* NoSQL: Add (micro-ish) benchmarks (apache#3006)

A project for JMH based benchmarks against NoSQL persistence.

* Helm chart: include configmap checksum in deployment annotations (apache#3023)

* fix(deps): update dependency ch.qos.logback:logback-classic to v1.5.21 (apache#3025)

* NoSQL: Realms handling (apache#3007)

Introduces handling for realms including realm-state management/transition.

The `RealmStore` implementation for NoSQL depends on CDI components, coming in a follo-up PR.

* Rename AccessConfig and AccessConfigProvider for clarity (apache#2883)

* rename AccessConfig for clarity

* rename getStorageAccessConfig() and add javadoc

* Refactor: improve and clean up Dockerfiles (apache#2957)

* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles

* Refactor: improve and clean up Dockerfiles

* Make StorageAccessConfigProvider request-scoped (apache#2974)

- add `StorageCredentialsVendor` as request-scoped wrapper around `PolarisCredentialVendor`
- make `FileIOFactory` request-scoped
- make `TaskFileIOSupplier` request-scoped

* Increase javadoc visibility in `nosql/realms` (apache#3029)

This is to fix javadoc error: `No public or protected classes found to document`

* NoSQL: Add correctness tests (apache#3027)

Verifies the correctness of concurrent commits, and big index handling.

These tests are intentionally _not_ part of the base-backend test suite for two reasons:
1. These tests do not run against the `Backend` interface but the `Persistence` interface, including commit and index logic.
2. These tests are intended to be runnable against a custom provisioned database cluster, not just tiny-ish test containers.

* NoSQL: Add maintenance API, SPI (apache#3028)

Maintenance operations include a bunch of tasks that are regularly executed against a backend database.

Types of maintenance operations include:
* Purging unreferenced objects and references within a catalog
* Purging whole catalogs that are marked to be purged
* Purging whole realms that are marked to be purged

Implementation added in a follow-up PR.

* Embrace request-scoped TokenBroker (apache#3024)

* Embrace request-scoped TokenBroker

`TokenBroker` and `CallContext` are both request-scoped, so instead of
passing the former into the latter, we can do this via the
`TokenBrokerFactory` and thus simplify the `TokenBroker` interface.

* fix(deps): update dependency io.smallrye:jandex to v3.5.2 (apache#3032)

* Fix monkey patching (apache#3016)

* chore(deps): update quay.io/keycloak/keycloak docker tag to v26.4.5 (apache#3034)

* chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.23-6.1762870925 (apache#3053)

* fix(deps): update dependency org.testcontainers:testcontainers-bom to v2.0.2 (apache#3054)

* chore(deps): update postgres docker tag to v18.1 (apache#3055)

* Add Polaris Community Meeting 2025-11-13 (apache#3060)

* Site: Rename menu "downloads" to "releases" (apache#2928)

* Update dependency software.amazon.awssdk:bom to v2.38.7 (apache#3065)

* Test-fix: Cleanup OPA test container on stop (apache#3041)

Quarkus takes care of reusing a test-resource across tests. The current behavior leaves the container around.

Plus some nit-fixes (deprecation + local var)

* Update dependency org.apache.commons:commons-lang3 to v3.20.0 (apache#3063)

* Build: ensure LICENSE/NOTICE is in all jars, always add pom-files to all jars (apache#3057)

There are a some inconsistencies between the different kinds of jars and the included information:
* LICENSE/NOTICE files are present in the "main" jar and in the sources jar, but not in the javadoc jar.
* The Maven pom.xml and pom.properties files are only present for release builds or when explicitly requested.
* "Additional" jar-manifest attributes that are only present in release builds.

This change fixes the three mentioned issues:
* Always include pom.xml and pom.properties in the built jar files.
* Always include the additional jar-manifest attributes, except the Git information, which would otherwise render the Gradle build cache ineffective.
* Include pom.xml + pom.properties + license/notice in literally all jar files.

The Gradle logic to include the license+notice+pom files has been simplified as well.

* Remove unused polarisEventListener field from IcebergCatalogHandler (apache#3045)

it was added in c3f5001 but then its
only usage was removed in d03c717

* fix(deps): update quarkus platform and group to v3.29.3 (apache#3052)

* Site: Add Open Policy Agent (OPA) as External Policy Decision Point (apache#3030)

Doc PR following up the introduction of OpaPolarisAuthorizer: apache#2680

* OPA: Tackle deprecation warnings (apache#3042)

Instead of suppressing the deprecations, this change updates the code a little bit to remove the mocks (except to create a non-nullable parameter).

* Use POJOs for OPA JSON schema construction and publish schema (apache#3031)

Co-authored-by: Robert Stupp <snazy@snazy.de>

* Use CDI for more test setups (apache#3040)

this avoids a bunch of redundant manual setup.

the important parts are establishing a `RealmContext` by calling
`QuarkusMock.installMockForType` and then populating `polarisContext`
from the injected `CallContext`.

* fix(deps): update dependency com.github.dasniko:testcontainers-keycloak to v4 (apache#3070)

* chore(deps): update actions/checkout digest to 93cb6ef (apache#3068)

* OPA: Fail fast when OPA bearer token file is unreadable (apache#3062)

* fix(deps): update immutables to v2.11.7 (apache#3072)

* Skip Hugo Site workflow on forks (apache#3056)

Forks usually don't have the "versioned-docs" tag and thus PRs against forks or rebasing the main branch on a fork currently always causes workflow failures.

* Fix warnings around TransactionWorkspaceMetaStoreManager (apache#3044)

- dont return `null` for interface methods that are `@Nonnull`
- fix wrong method name parameters
- dont annotate void methods as `@Nonnull`

* NoSQL: Add CDI/common+testing + necessary nosql-store implementations (apache#3035)

Adds common and test-specific CDI functionality. Requires the NoSQL store implementations `:polaris-persistence-nosql-realms-store-nosql` and `:polaris-nodes-store-nosql`.

Those modules have cross-project dependencies for test purposes, hence those are all contained in this PR.

CDI for Quarkus will be added in a follow-up.

* Automate the release guide - Take 2 - Github workflows (apache#2383)

The release automation is simplified to four GitHub workflows that just require the really mandatory user input: the version number.
1. workflow: Trigger the creation of the release branch
2. workflow: Upgrade the release branch with the version and build the the final change-log for that version
3. workflow: Build the RC artifacts from the release branch and push those to the various staging repositories
4. workflow: Eventually release the artifacts.

See also the [email announcement](https://lists.apache.org/thread/d0smz07gnr509yj5dc6omo3cvkf1pnh7).

---------

Co-authored-by: Robert Stupp <snazy@snazy.de>

* Update actions/checkout digest to 93cb6ef (apache#3082)

* NoSQL: adapt to conflicting changes in main

* Last merged commit 8ccddc5

---------

Co-authored-by: Michael Collado <40346148+collado-mike@users.noreply.github.com>
Co-authored-by: cccs-cat001 <56204545+cccs-cat001@users.noreply.github.com>
Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com>
Co-authored-by: Alexandre Dutra <adutra@apache.org>
Co-authored-by: Mend Renovate <bot@renovateapp.com>
Co-authored-by: Nuoya Jiang <98131931+CodingBangboo@users.noreply.github.com>
Co-authored-by: Yong Zheng <yongzheng0809@gmail.com>
Co-authored-by: Christopher Lambert <xn137@gmx.de>
Co-authored-by: JB Onofré <jbonofre@apache.org>
Co-authored-by: Yufei Gu <yufei@apache.org>
Co-authored-by: Sung Yun <107272191+sungwy@users.noreply.github.com>
Co-authored-by: Pierre Laporte <pierre@pingtimeout.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants