Prosecco

[mk2023]

These are the tests for Milestone 2:
ConnectorConfig
DataConnect (init)
Public Interface: client()
_DataConnectService

[gemini-code-assist]

Code Review

This pull request introduces the new dataconnect module, including the ConnectorConfig and DataConnect classes, along with a comprehensive test suite. However, the implementation is currently incomplete and will fail the tests. Specifically, the DataConnect class is missing the public app and config properties, the client function is left as a stub, and the _DataConnectService class is completely missing. Additionally, the validation in ConnectorConfig needs to be updated to explicitly check for string types to prevent invalid types from bypassing the checks.

[gemini-code-assist]

The DataConnect class is missing the public read-only properties app and config which are accessed by the tests (e.g., data_connect_instance.app and data_connect_instance.config) and expected as part of the public API. Let's add these properties.

Suggested change

	class DataConnect:
	def __init__(self, app: App, config: ConnectorConfig) -> None:
	"""Initializes a DataConnect client instance"""
	self._app: App = app
	self._config = config
	class DataConnect:
	def __init__(self, app: App, config: ConnectorConfig) -> None:
	"""Initializes a DataConnect client instance"""
	self._app: App = app
	self._config = config
	@property
	def app(self) -> App:
	return self._app
	@property
	def config(self) -> ConnectorConfig:
	return self._config

[gemini-code-assist]

The client function is currently just a stub with a docstring and is missing its implementation. Additionally, the internal _DataConnectService class, which manages client caching and is used by client and verified in the tests, is completely missing. Let's implement both _DataConnectService and the client factory function.

class _DataConnectService:
    def __init__(self, app: App) -> None:
        self._app = app
        self._clients: Dict[ConnectorConfig, DataConnect] = {}

    def get_client(self, config: ConnectorConfig) -> DataConnect:
        if not isinstance(config, ConnectorConfig):
            raise ValueError("Config must be of type firebase_admin.dataconnect.ConnectorConfig")
        if config not in self._clients:
            self._clients[config] = DataConnect(self._app, config)
        return self._clients[config]


def client(config: ConnectorConfig, app: Optional[App] = None) -> DataConnect:
    """Returns a DataConnect client for the specified configuration"""
    if not isinstance(config, ConnectorConfig):
        raise ValueError("Config must be of type firebase_admin.dataconnect.ConnectorConfig")
    if app is not None and not isinstance(app, App):
        raise ValueError("App must be of type firebase_admin.App")

    from firebase_admin import _utils
    service = _utils.get_app_service(app, "_data_connect_service", _DataConnectService)
    return service.get_client(config)

[gemini-code-assist]

The __post_init__ method only checks if the attributes are falsy (e.g., not self.location). However, if an invalid type like an integer (e.g., 123) is passed, not 123 evaluates to False, and the validation is bypassed. To ensure proper type safety and match the test expectations in test_connector_config_invalid_types, we should explicitly verify that the fields are non-empty strings.

Suggested change

	def __post_init__(self):
	if not self.service_id:
	raise ValueError("service_id cannot be empty")
	if not self.location:
	raise ValueError("location cannot be empty")
	if not self.connector:
	raise ValueError("connector cannot be empty")
	def __post_init__(self):
	if not isinstance(self.service_id, str) or not self.service_id:
	raise ValueError("service_id cannot be empty")
	if not isinstance(self.location, str) or not self.location:
	raise ValueError("location cannot be empty")
	if not isinstance(self.connector, str) or not self.connector:
	raise ValueError("connector cannot be empty")