Created script just for calling the login/logout endpoint#213
Created script just for calling the login/logout endpoint#213milanmajchrak wants to merge 1 commit intomainfrom
Conversation
WalkthroughA new script, Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)tools/policy_and_auth/login_requests.py✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tools/policy_and_auth/login_requests.py (2)
5-8: Consider making path resolution more robust.The current path manipulation using relative paths (
../../) can be brittle if the script is moved or executed from different working directories. Consider using more robust path resolution or packaging the dependencies properly.-_this_dir = os.path.dirname(os.path.abspath(__file__)) -path_to_dspace_lib = os.path.join(_this_dir, "../../libs/dspace-rest-python") -sys.path.insert(0, path_to_dspace_lib) -sys.path.insert(0, os.path.join(_this_dir, "../../src")) +_this_dir = os.path.dirname(os.path.abspath(__file__)) +_root_dir = os.path.join(_this_dir, "../..") +path_to_dspace_lib = os.path.join(_root_dir, "libs/dspace-rest-python") +path_to_src = os.path.join(_root_dir, "src") + +if os.path.exists(path_to_dspace_lib): + sys.path.insert(0, path_to_dspace_lib) +if os.path.exists(path_to_src): + sys.path.insert(0, path_to_src)
26-29: Complete the logout implementation or remove the commented code.The commented logout functionality suggests incomplete implementation. Either implement it properly or remove the dead code to keep the script clean.
If logout testing is needed, implement it properly:
- # Call logout every 5th request - # if i % 5 == 0: - # dspace_be.client.logout() - # print("Logged out") + # Call logout every 5th request for testing session management + logout_interval = int(os.environ.get("LOGOUT_INTERVAL", "0")) + if logout_interval > 0 and i % logout_interval == 0: + try: + dspace_be.client.logout() + _logger.info(f"Logged out after {i} attempts") + except Exception as e: + _logger.error(f"Logout failed: {e}")Or remove it entirely if not needed:
- # Call logout every 5th request - # if i % 5 == 0: - # dspace_be.client.logout() - # print("Logged out")
| env = update_settings(settings.env, project_settings.settings) | ||
| init_logging(_logger, env["log_file"]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required environment variables.
The script assumes required credentials exist in the environment but doesn't validate them. This could lead to unclear error messages if configuration is missing.
env = update_settings(settings.env, project_settings.settings)
init_logging(_logger, env["log_file"])
+# Validate required environment variables
+required_keys = ["backend.endpoint", "backend.user", "backend.password"]
+missing_keys = []
+for key in required_keys:
+ keys = key.split('.')
+ value = env
+ try:
+ for k in keys:
+ value = value[k]
+ if not value:
+ missing_keys.append(key)
+ except (KeyError, TypeError):
+ missing_keys.append(key)
+
+if missing_keys:
+ _logger.error(f"Missing required environment variables: {missing_keys}")
+ sys.exit(1)Also applies to: 21-21
🤖 Prompt for AI Agents
In tools/policy_and_auth/login_requests.py at lines 15-16 and line 21, the code
uses environment variables without validating their presence, which can cause
unclear errors. Add explicit checks after loading the environment to verify that
all required environment variables are set. If any are missing, raise a clear
exception or log an error indicating which variables are absent before
proceeding with the rest of the script.
| dspace_be = dspace.rest(env["backend"]["endpoint"], env["backend"]["user"], env["backend"]["password"], True) | ||
| for i in range(1, 10): | ||
| _logger.info(f"Authenticating {i}") | ||
| dspace_be.client.authenticate() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and make the loop count configurable.
The authentication loop lacks error handling and uses a hard-coded count. This could cause the script to fail silently or make it difficult to customize for different testing scenarios.
- dspace_be = dspace.rest(env["backend"]["endpoint"], env["backend"]["user"], env["backend"]["password"], True)
- for i in range(1, 10):
- _logger.info(f"Authenticating {i}")
- dspace_be.client.authenticate()
+ try:
+ dspace_be = dspace.rest(env["backend"]["endpoint"], env["backend"]["user"], env["backend"]["password"], True)
+
+ # Make loop count configurable via environment variable
+ loop_count = int(os.environ.get("AUTH_LOOP_COUNT", "9"))
+
+ for i in range(1, loop_count + 1):
+ _logger.info(f"Authenticating attempt {i}/{loop_count}")
+ try:
+ dspace_be.client.authenticate()
+ _logger.info(f"Authentication attempt {i} successful")
+ except Exception as e:
+ _logger.error(f"Authentication attempt {i} failed: {e}")
+
+ except Exception as e:
+ _logger.error(f"Failed to initialize DSpace backend: {e}")
+ sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dspace_be = dspace.rest(env["backend"]["endpoint"], env["backend"]["user"], env["backend"]["password"], True) | |
| for i in range(1, 10): | |
| _logger.info(f"Authenticating {i}") | |
| dspace_be.client.authenticate() | |
| try: | |
| dspace_be = dspace.rest( | |
| env["backend"]["endpoint"], | |
| env["backend"]["user"], | |
| env["backend"]["password"], | |
| True | |
| ) | |
| # Make loop count configurable via environment variable | |
| loop_count = int(os.environ.get("AUTH_LOOP_COUNT", "9")) | |
| for i in range(1, loop_count + 1): | |
| _logger.info(f"Authenticating attempt {i}/{loop_count}") | |
| try: | |
| dspace_be.client.authenticate() | |
| _logger.info(f"Authentication attempt {i} successful") | |
| except Exception as e: | |
| _logger.error(f"Authentication attempt {i} failed: {e}") | |
| except Exception as e: | |
| _logger.error(f"Failed to initialize DSpace backend: {e}") | |
| sys.exit(1) |
🤖 Prompt for AI Agents
In tools/policy_and_auth/login_requests.py around lines 21 to 24, the
authentication loop uses a hard-coded count of 9 and lacks error handling.
Modify the code to make the loop count configurable via a parameter or
environment variable, and wrap the authentication call in a try-except block to
catch and log any exceptions during authentication attempts.
1 participant
Problem description
Related to this issue: https://github.com/dataquest-dev/dspace-customers/issues/156
Summary by CodeRabbit