mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-14 02:15:06 +00:00
Address review: model metadata, serialized admin saves, member account choice
- models.py: connector_policies.enabled is the nullable column; the earlier edit had loosened user_custom_models.enabled instead. A test pins both against their migrations. - Admin > Connectors saves one change at a time, so an older snapshot can no longer overwrite a newer one. - In member mode, a member with several connected accounts of the same service uses the one they used (or connected) most recently, not the oldest.
This commit is contained in:
1 parent
ca8fb94777
commit
7c6154749a
6 files changed
+110
-21
No files matched your search
@@ -9,6 +9,7 @@ credentials; callers read them from the resolved row through
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from datetime import datetime, timezone
|
||||
import logging
|
||||
from dataclasses import dataclass
|
||||
from typing import Optional
|
||||
@@ -107,17 +108,34 @@ def resolve_connection(resource: dict, invoker_user_id: Optional[str]) -> Option
|
||||
|
||||
|
||||
def _member_connection(repo: ConnectorSessionsRepository, owned: Optional[dict], invoker: str) -> Optional[dict]:
|
||||
"""The invoker's own connection to the service the owner's connection is for."""
|
||||
"""The invoker's own connection to the service the owner's connection is for.
|
||||
|
||||
A member with several connected accounts of that service gets the one
|
||||
they used most recently (then the most recently connected): the account
|
||||
they are working in, and the one a "Connect to continue" just added.
|
||||
"""
|
||||
if owned is None:
|
||||
return None
|
||||
for row in repo.list_for_user(invoker):
|
||||
if row.get("provider") != owned.get("provider"):
|
||||
continue
|
||||
if (row.get("server_url") or "") != (owned.get("server_url") or ""):
|
||||
continue
|
||||
if service.normalize_status(row) == service.STATUS_CONNECTED:
|
||||
return row
|
||||
return None
|
||||
candidates = [
|
||||
row for row in repo.list_for_user(invoker)
|
||||
if row.get("provider") == owned.get("provider")
|
||||
and (row.get("server_url") or "") == (owned.get("server_url") or "")
|
||||
and service.normalize_status(row) == service.STATUS_CONNECTED
|
||||
]
|
||||
if not candidates:
|
||||
return None
|
||||
|
||||
def when(value) -> datetime:
|
||||
if isinstance(value, str):
|
||||
value = datetime.fromisoformat(value)
|
||||
if not isinstance(value, datetime):
|
||||
return datetime.min.replace(tzinfo=timezone.utc)
|
||||
return value if value.tzinfo else value.replace(tzinfo=timezone.utc)
|
||||
|
||||
def recency(row: dict) -> tuple:
|
||||
return tuple(when(row.get(field)) for field in ("last_used_at", "updated_at", "created_at"))
|
||||
|
||||
return max(candidates, key=recency)
|
||||
|
||||
|
||||
def audit_delegation(resolved: ResolvedConnection, *, invoker: Optional[str], resource_type: str,
|
||||
|
||||
@@ -419,8 +419,7 @@ user_custom_models_table = Table(
|
||||
# docsgpt.security.encryption.encrypt_credentials.
|
||||
Column("api_key_encrypted", Text, nullable=False),
|
||||
Column("capabilities", JSONB, nullable=False, server_default="{}"),
|
||||
# NULL: on when the connector has its server settings (see connectors.service).
|
||||
Column("enabled", Boolean),
|
||||
Column("enabled", Boolean, nullable=False, server_default="true"),
|
||||
Column("created_at", DateTime(timezone=True), nullable=False, server_default=func.now()),
|
||||
Column("updated_at", DateTime(timezone=True), nullable=False, server_default=func.now()),
|
||||
)
|
||||
@@ -616,7 +615,8 @@ connector_policies_table = Table(
|
||||
"connector_policies",
|
||||
metadata,
|
||||
Column("connector_key", Text, primary_key=True),
|
||||
Column("enabled", Boolean, nullable=False, server_default="true"),
|
||||
# NULL: on when the connector has its server settings (see connectors.service).
|
||||
Column("enabled", Boolean),
|
||||
Column("credential_mode", Text, nullable=False, server_default="choose"),
|
||||
Column("updated_by", Text),
|
||||
Column("updated_at", DateTime(timezone=True), nullable=False, server_default=func.now()),
|
||||
|
||||
@@ -165,6 +165,43 @@ describe('Admin Connectors', () => {
|
||||
expect(container.querySelectorAll('tbody tr')).toHaveLength(2);
|
||||
});
|
||||
|
||||
it('saves one change at a time so a late response cannot win', async () => {
|
||||
getAdmin.mockResolvedValue(payload());
|
||||
const pending: ((value: unknown) => void)[] = [];
|
||||
updateAdmin.mockImplementation(
|
||||
() => new Promise((resolve) => pending.push(resolve)),
|
||||
);
|
||||
await render();
|
||||
const toggle = () =>
|
||||
container.querySelector<HTMLButtonElement>(
|
||||
'[aria-label="Google Drive enabled"]',
|
||||
)!;
|
||||
await act(async () => toggle().click());
|
||||
await act(async () =>
|
||||
container.querySelector<HTMLButtonElement>('#allow-custom-mcp')!.click(),
|
||||
);
|
||||
// The second save waits for the first.
|
||||
expect(updateAdmin).toHaveBeenCalledTimes(1);
|
||||
await act(async () =>
|
||||
pending[0](payload({ connectors: [connector({ enabled: false })] })),
|
||||
);
|
||||
expect(updateAdmin).toHaveBeenCalledTimes(2);
|
||||
await act(async () =>
|
||||
pending[1](
|
||||
payload({
|
||||
connectors: [connector({ enabled: false })],
|
||||
allow_custom_mcp: false,
|
||||
}),
|
||||
),
|
||||
);
|
||||
expect(toggle().getAttribute('aria-checked')).toBe('false');
|
||||
expect(
|
||||
container
|
||||
.querySelector('#allow-custom-mcp')!
|
||||
.getAttribute('aria-checked'),
|
||||
).toBe('false');
|
||||
});
|
||||
|
||||
it('offers a retry when loading fails', async () => {
|
||||
getAdmin.mockRejectedValue(new Error('offline'));
|
||||
await render();
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { ExternalLink, Info, TriangleAlert } from 'lucide-react';
|
||||
import { useCallback, useEffect, useState } from 'react';
|
||||
import { useCallback, useEffect, useRef, useState } from 'react';
|
||||
import { useDispatch, useSelector } from 'react-redux';
|
||||
|
||||
import connectorsService from '../api/services/connectorsService';
|
||||
@@ -177,7 +177,10 @@ export default function Connectors() {
|
||||
load();
|
||||
}, [load]);
|
||||
|
||||
const save = async (body: {
|
||||
// Saves run one after another: each response is a full snapshot, so an
|
||||
// older one arriving last would otherwise put back a stale policy.
|
||||
const saveQueue = useRef<Promise<void>>(Promise.resolve());
|
||||
const save = (body: {
|
||||
policies?: Record<string, { enabled?: boolean; credential_mode?: Policy }>;
|
||||
allow_custom_mcp?: boolean;
|
||||
}) => {
|
||||
@@ -188,13 +191,16 @@ export default function Connectors() {
|
||||
message: 'Could not save the change.',
|
||||
}),
|
||||
);
|
||||
try {
|
||||
const next = await connectorsService.updateAdmin(body, token);
|
||||
if (next?.success) setData(next);
|
||||
else failed();
|
||||
} catch {
|
||||
failed();
|
||||
}
|
||||
saveQueue.current = saveQueue.current.then(async () => {
|
||||
try {
|
||||
const next = await connectorsService.updateAdmin(body, token);
|
||||
if (next?.success) setData(next);
|
||||
else failed();
|
||||
} catch {
|
||||
failed();
|
||||
}
|
||||
});
|
||||
return saveQueue.current;
|
||||
};
|
||||
|
||||
if (data === null && loading) return <LoadingState fill="block" />;
|
||||
|
||||
@@ -86,6 +86,23 @@ class TestResolution:
|
||||
assert resolved.connection_id == bobs
|
||||
assert resolved.delegated is False
|
||||
|
||||
def test_member_with_several_accounts_uses_the_one_last_used(self, pg_conn):
|
||||
from docsgpt.connectors.resolve import resolve_connection
|
||||
|
||||
owner = _connection(pg_conn)
|
||||
older = _connection(pg_conn, user="bob")
|
||||
pg_conn.execute(text(
|
||||
"UPDATE connector_sessions SET account_label = 'bob-home', last_used_at = now() - interval '1 day' "
|
||||
"WHERE id = CAST(:i AS uuid)"
|
||||
), {"i": older})
|
||||
newer = _connection(pg_conn, user="bob")
|
||||
pg_conn.execute(text(
|
||||
"UPDATE connector_sessions SET last_used_at = now() WHERE id = CAST(:i AS uuid)"
|
||||
), {"i": newer})
|
||||
with _service_db(pg_conn):
|
||||
resolved = resolve_connection(_tool(owner, mode="member"), "bob")
|
||||
assert resolved.available and resolved.connection_id == newer
|
||||
|
||||
def test_member_mode_without_own_connection_is_unavailable(self, pg_conn):
|
||||
from docsgpt.connectors.resolve import resolve_connection
|
||||
|
||||
|
||||
@@ -384,3 +384,14 @@ class TestMigration0038Credentials:
|
||||
).scalar()
|
||||
assert read_secrets(drive)["token_info"]["access_token"] == "plain-at"
|
||||
assert count == 1
|
||||
|
||||
|
||||
def test_model_metadata_matches_the_policy_and_custom_model_columns():
|
||||
"""The SQLAlchemy tables mirror the migrations for both ``enabled`` columns."""
|
||||
from docsgpt.storage.db.models import connector_policies_table, user_custom_models_table
|
||||
|
||||
policy_enabled = connector_policies_table.c.enabled
|
||||
assert policy_enabled.nullable is True and policy_enabled.server_default is None
|
||||
model_enabled = user_custom_models_table.c.enabled
|
||||
assert model_enabled.nullable is False
|
||||
assert str(model_enabled.server_default.arg) == "true"
|
||||
Reference in new issue
Block a user