Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing SSRC Typos and Minor Bugs #841

Draft
wants to merge 8 commits into
base: ssrc
Choose a base branch
from
Draft

Fixing SSRC Typos and Minor Bugs #841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e377491
Adding script for Python Bug Bash
Dec 4, 2024
f9f0635
Changes for percent comparison
Dec 5, 2024
5f6f03e
Fixing semantic version issues with invalid version
Dec 8, 2024
ba678a9
Config values must retrun default values from invalid get operations
Dec 8, 2024
84ebe4c
Fixing typos
Dec 18, 2024
12fb2f1
Adding requirements set is master branch
Dec 18, 2024
d276c3e
Updating tolerance for percentage evaluation
Dec 18, 2024
f22ef5b
Removing dependency changes from fix branch
Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 38 additions & 30 deletions firebase_admin/remote_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Copyright 2017 Google Inc.
# Copyright 2024 Google Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -197,7 +197,7 @@ def get_float(self, key):
return self.get_value(key).as_float()

def get_value(self, key):
if self._config_values[key]:
Copy link
Member

Choose a reason for hiding this comment

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

Could we add type hints for all the functions where possible, please?
We are trying to add type hints to any new code. For example, see:

firebase-admin-python/firebase_admin/app_check.py

Line 73 in 43ab91e

def verify_token(self, token: str) -> Dict[str, Any]:

Copy link
Member

Choose a reason for hiding this comment

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

Just noticed that you have added typehints to return types in most of the public functions. Thanks!

if key in self._config_values:
return self._config_values[key]
return _Value('static')
Copy link
Member

Choose a reason for hiding this comment

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

You might also be able to just do self._config_values.get(key, _Value('static')) here


Expand Down Expand Up @@ -421,11 +421,11 @@ def evaluate_percent_condition(self, percent_condition,

hash64 = self.hash_seeded_randomization_id(string_to_hash)
instance_micro_percentile = hash64 % (100 * 1000000)
if percent_operator == PercentConditionOperator.LESS_OR_EQUAL:
if percent_operator == PercentConditionOperator.LESS_OR_EQUAL.value:
return instance_micro_percentile <= norm_micro_percent
if percent_operator == PercentConditionOperator.GREATER_THAN:
if percent_operator == PercentConditionOperator.GREATER_THAN.value:
return instance_micro_percentile > norm_micro_percent
if percent_operator == PercentConditionOperator.BETWEEN:
if percent_operator == PercentConditionOperator.BETWEEN.value:
return norm_percent_lower_bound < instance_micro_percentile <= norm_percent_upper_bound
logger.warning("Unknown percent operator: %s", percent_operator)
return False
Expand Down Expand Up @@ -454,10 +454,10 @@ def evaluate_custom_signal_condition(self, custom_signal_condition,
Returns:
True if the condition is met, False otherwise.
"""
custom_signal_operator = custom_signal_condition.get('custom_signal_operator') or {}
custom_signal_key = custom_signal_condition.get('custom_signal_key') or {}
custom_signal_operator = custom_signal_condition.get('customSignalOperator') or {}
custom_signal_key = custom_signal_condition.get('customSignalKey') or {}
target_custom_signal_values = (
custom_signal_condition.get('target_custom_signal_values') or {})
custom_signal_condition.get('targetCustomSignalValues') or {})

if not all([custom_signal_operator, custom_signal_key, target_custom_signal_values]):
logger.warning("Missing operator, key, or target values for custom signal condition.")
Expand All @@ -471,71 +471,71 @@ def evaluate_custom_signal_condition(self, custom_signal_condition,
logger.warning("Custom signal value not found in context: %s", custom_signal_key)
return False

if custom_signal_operator == CustomSignalOperator.STRING_CONTAINS:
if custom_signal_operator == CustomSignalOperator.STRING_CONTAINS.value:
return self._compare_strings(target_custom_signal_values,
actual_custom_signal_value,
lambda target, actual: target in actual)
if custom_signal_operator == CustomSignalOperator.STRING_DOES_NOT_CONTAIN:
if custom_signal_operator == CustomSignalOperator.STRING_DOES_NOT_CONTAIN.value:
return not self._compare_strings(target_custom_signal_values,
actual_custom_signal_value,
lambda target, actual: target in actual)
if custom_signal_operator == CustomSignalOperator.STRING_EXACTLY_MATCHES:
if custom_signal_operator == CustomSignalOperator.STRING_EXACTLY_MATCHES.value:
return self._compare_strings(target_custom_signal_values,
actual_custom_signal_value,
lambda target, actual: target.strip() == actual.strip())
if custom_signal_operator == CustomSignalOperator.STRING_CONTAINS_REGEX:
if custom_signal_operator == CustomSignalOperator.STRING_CONTAINS_REGEX.value:
return self._compare_strings(target_custom_signal_values,
actual_custom_signal_value,
re.search)

# For numeric operators only one target value is allowed.
if custom_signal_operator == CustomSignalOperator.NUMERIC_LESS_THAN:
if custom_signal_operator == CustomSignalOperator.NUMERIC_LESS_THAN.value:
return self._compare_numbers(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r < 0)
if custom_signal_operator == CustomSignalOperator.NUMERIC_LESS_EQUAL:
if custom_signal_operator == CustomSignalOperator.NUMERIC_LESS_EQUAL.value:
return self._compare_numbers(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r <= 0)
if custom_signal_operator == CustomSignalOperator.NUMERIC_EQUAL:
if custom_signal_operator == CustomSignalOperator.NUMERIC_EQUAL.value:
return self._compare_numbers(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r == 0)
if custom_signal_operator == CustomSignalOperator.NUMERIC_NOT_EQUAL:
if custom_signal_operator == CustomSignalOperator.NUMERIC_NOT_EQUAL.value:
return self._compare_numbers(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r != 0)
if custom_signal_operator == CustomSignalOperator.NUMERIC_GREATER_THAN:
if custom_signal_operator == CustomSignalOperator.NUMERIC_GREATER_THAN.value:
return self._compare_numbers(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r > 0)
if custom_signal_operator == CustomSignalOperator.NUMERIC_GREATER_EQUAL:
if custom_signal_operator == CustomSignalOperator.NUMERIC_GREATER_EQUAL.value:
return self._compare_numbers(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r >= 0)

# For semantic operators only one target value is allowed.
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_LESS_THAN:
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_LESS_THAN.value:
return self._compare_semantic_versions(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r < 0)
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_LESS_EQUAL:
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_LESS_EQUAL.value:
return self._compare_semantic_versions(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r <= 0)
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_EQUAL:
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_EQUAL.value:
return self._compare_semantic_versions(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r == 0)
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_NOT_EQUAL:
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_NOT_EQUAL.value:
return self._compare_semantic_versions(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r != 0)
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_GREATER_THAN:
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_GREATER_THAN.value:
return self._compare_semantic_versions(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r > 0)
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_GREATER_EQUAL:
if custom_signal_operator == CustomSignalOperator.SEMANTIC_VERSION_GREATER_EQUAL.value:
return self._compare_semantic_versions(target_custom_signal_values[0],
actual_custom_signal_value,
lambda r: r >= 0)
Expand Down Expand Up @@ -593,9 +593,9 @@ def _compare_versions(self, version1, version2, predicate_fn) -> bool:
"""Compares two semantic version strings.

Args:
version1: The first semantic version string.
version2: The second semantic version string.
predicate_fn: A function that takes an integer and returns a boolean.
version1: The first semantic version string.
version2: The second semantic version string.
predicate_fn: A function that takes an integer and returns a boolean.

Copy link
Member

Choose a reason for hiding this comment

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

Let's get a TW review on these docstrings

Returns:
bool: The result of the predicate function.
Expand All @@ -608,6 +608,8 @@ def _compare_versions(self, version1, version2, predicate_fn) -> bool:
v2_parts.extend([0] * (max_length - len(v2_parts)))

for part1, part2 in zip(v1_parts, v2_parts):
if any((part1 < 0, part2 < 0)):
raise ValueError
if part1 < part2:
return predicate_fn(-1)
if part1 > part2:
Expand Down Expand Up @@ -674,7 +676,7 @@ def as_string(self) -> str:
"""Returns the value as a string."""
if self.source == 'static':
return self.DEFAULT_VALUE_FOR_STRING
return self.value
return str(self.value)

def as_boolean(self) -> bool:
"""Returns the value as a boolean."""
Expand All @@ -686,13 +688,19 @@ def as_int(self) -> float:
"""Returns the value as a number."""
if self.source == 'static':
return self.DEFAULT_VALUE_FOR_INTEGER
return self.value
try:
return int(self.value)
except ValueError:
return self.DEFAULT_VALUE_FOR_INTEGER

def as_float(self) -> float:
"""Returns the value as a number."""
if self.source == 'static':
return self.DEFAULT_VALUE_FOR_FLOAT_NUMBER
return float(self.value)
try:
return float(self.value)
except ValueError:
return self.DEFAULT_VALUE_FOR_FLOAT_NUMBER

def get_source(self) -> ValueSource:
"""Returns the source of the value."""
Expand Down
Loading
Loading