Cleanup & improved typing #69
Conversation
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a number of cleanups and improvements, such as removing deprecated code, improving type hints, and refactoring the KAS class out of the SDK class for better separation of concerns. The changes are generally good, but I've found a few issues, mostly in the new test files. There's a leftover debug print statement, a bug in the SDK builder logic for tests, and a call to a non-existent method in a new integration test. I've also suggested a small refactoring to reduce code duplication in a test helper file. Please review the comments for details.
| sdk = ( | ||
| SDKBuilder() | ||
| .set_platform_endpoint(CONFIG_TDF.OPENTDF_PLATFORM_URL) | ||
| .set_issuer_endpoint(CONFIG_TDF.OPENTDF_KEYCLOAK_HOST) | ||
| .use_insecure_skip_verify(CONFIG_TDF.INSECURE_SKIP_VERIFY) | ||
| .build() | ||
| ) |
There was a problem hiding this comment.
It looks like the auth_token is not being passed to the SDKBuilder in the https branch of this conditional. This is inconsistent with the http branch and will likely cause authentication issues. The SDKBuilder should be initialized with auth_token=user_token here as well.
| sdk = ( | |
| SDKBuilder() | |
| .set_platform_endpoint(CONFIG_TDF.OPENTDF_PLATFORM_URL) | |
| .set_issuer_endpoint(CONFIG_TDF.OPENTDF_KEYCLOAK_HOST) | |
| .use_insecure_skip_verify(CONFIG_TDF.INSECURE_SKIP_VERIFY) | |
| .build() | |
| ) | |
| sdk = ( | |
| SDKBuilder(auth_token=user_token) | |
| .set_platform_endpoint(CONFIG_TDF.OPENTDF_PLATFORM_URL) | |
| .set_issuer_endpoint(CONFIG_TDF.OPENTDF_KEYCLOAK_HOST) | |
| .use_insecure_skip_verify(CONFIG_TDF.INSECURE_SKIP_VERIFY) | |
| .build() | |
| ) |
| # tdf_reader = sdk.load_tdf(infile.read(), reader_config) | ||
| tdf_reader = sdk.load_tdf_without_config(infile.read()) |
There was a problem hiding this comment.
The method load_tdf_without_config does not exist on the SDK object. The correct method is sdk.load_tdf, which requires a TDFReaderConfig instance. Although this test is skipped, it's best to fix this to avoid confusion and make it easier to enable the test later.
| # tdf_reader = sdk.load_tdf(infile.read(), reader_config) | |
| tdf_reader = sdk.load_tdf_without_config(infile.read()) | |
| reader_config = TDFReaderConfig(attributes=_test_attributes) | |
| tdf_reader = sdk.load_tdf(infile.read(), reader_config) |
| Returns: | ||
| Unwrapped key as bytes | ||
| """ | ||
| print("Pause") |
There was a problem hiding this comment.
This print() statement appears to be a leftover from debugging. It should be removed before merging.
| def get_sdk() -> SDK: | ||
| if CONFIG_TDF.OPENTDF_PLATFORM_URL.startswith("http://"): | ||
| sdk = ( | ||
| SDKBuilder() | ||
| .set_platform_endpoint(CONFIG_TDF.OPENTDF_PLATFORM_URL) | ||
| .set_issuer_endpoint(CONFIG_TDF.OPENTDF_KEYCLOAK_HOST) | ||
| .client_secret( | ||
| CONFIG_TDF.OPENTDF_CLIENT_ID, | ||
| CONFIG_TDF.OPENTDF_CLIENT_SECRET, | ||
| ) | ||
| .use_insecure_plaintext_connection(True) | ||
| .use_insecure_skip_verify(CONFIG_TDF.INSECURE_SKIP_VERIFY) | ||
| .build() | ||
| ) | ||
| elif CONFIG_TDF.OPENTDF_PLATFORM_URL.startswith("https://"): | ||
| sdk = ( | ||
| SDKBuilder() | ||
| .set_platform_endpoint(CONFIG_TDF.OPENTDF_PLATFORM_URL) | ||
| .set_issuer_endpoint(CONFIG_TDF.OPENTDF_KEYCLOAK_HOST) | ||
| .client_secret( | ||
| CONFIG_TDF.OPENTDF_CLIENT_ID, | ||
| CONFIG_TDF.OPENTDF_CLIENT_SECRET, | ||
| ) | ||
| .use_insecure_skip_verify(CONFIG_TDF.INSECURE_SKIP_VERIFY) | ||
| .build() | ||
| ) | ||
| else: | ||
| raise ValueError( | ||
| f"Invalid platform URL: {CONFIG_TDF.OPENTDF_PLATFORM_URL}. " | ||
| "It must start with 'http://' or 'https://'." | ||
| ) | ||
|
|
||
| return sdk |
There was a problem hiding this comment.
There's some code duplication in get_sdk between the http:// and https:// branches. This could be refactored to make the code more concise and maintainable. You could create the SDKBuilder instance with all common calls, and then conditionally call use_insecure_plaintext_connection(True) if the URL starts with http://.
No description provided.