Skip to content

fix: Application access restrictions are not separately counted for each application #1937

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

Merged
shaohuzhang1 merged 1 commit into from
Dec 30, 2024
Merged

fix: Application access restrictions are not separately counted for each application #1937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 9 additions & 7 deletions apps/application/chat_pipeline/step/chat_step/impl/base_chat_step.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,11 @@
from setting.models_provider.tools import get_model_instance_by_model_user_id


def add_access_num(client_id=None, client_type=None):
if client_type == AuthenticationType.APPLICATION_ACCESS_TOKEN.value:
application_public_access_client = QuerySet(ApplicationPublicAccessClient).filter(id=client_id).first()
def add_access_num(client_id=None, client_type=None, application_id=None):
if client_type == AuthenticationType.APPLICATION_ACCESS_TOKEN.value and application_id is not None:
application_public_access_client = (QuerySet(ApplicationPublicAccessClient).filter(client_id=client_id,
application_id=application_id)
.first())
if application_public_access_client is not None:
application_public_access_client.access_num = application_public_access_client.access_num + 1
application_public_access_client.intraday_access_num = application_public_access_client.intraday_access_num + 1
Expand Down Expand Up @@ -90,14 +92,14 @@ def event_content(response,
request_token, response_token,
{'node_is_end': True, 'view_type': 'many_view',
'node_type': 'ai-chat-node'})
add_access_num(client_id, client_type)
add_access_num(client_id, client_type, manage.context.get('application_id'))
except Exception as e:
logging.getLogger("max_kb_error").error(f'{str(e)}:{traceback.format_exc()}')
all_text = '异常' + str(e)
write_context(step, manage, 0, 0, all_text)
post_response_handler.handler(chat_id, chat_record_id, paragraph_list, problem_text,
all_text, manage, step, padding_problem_text, client_id)
add_access_num(client_id, client_type)
add_access_num(client_id, client_type, manage.context.get('application_id'))
yield manage.get_base_to_response().to_stream_chunk_response(chat_id, str(chat_record_id), all_text,
'ai-chat-node',
[], True, 0, 0,
Expand Down Expand Up @@ -241,7 +243,7 @@ def execute_block(self, message_list: List[BaseMessage],
write_context(self, manage, request_token, response_token, chat_result.content)
post_response_handler.handler(chat_id, chat_record_id, paragraph_list, problem_text,
chat_result.content, manage, self, padding_problem_text, client_id)
add_access_num(client_id, client_type)
add_access_num(client_id, client_type, manage.context.get('application_id'))
return manage.get_base_to_response().to_block_response(str(chat_id), str(chat_record_id),
chat_result.content, True,
request_token, response_token)
Expand All @@ -250,6 +252,6 @@ def execute_block(self, message_list: List[BaseMessage],
write_context(self, manage, 0, 0, all_text)
post_response_handler.handler(chat_id, chat_record_id, paragraph_list, problem_text,
all_text, manage, self, padding_problem_text, client_id)
add_access_num(client_id, client_type)
add_access_num(client_id, client_type, manage.context.get('application_id'))
return manage.get_base_to_response().to_block_response(str(chat_id), str(chat_record_id), all_text, True, 0,
0, _status=status.HTTP_500_INTERNAL_SERVER_ERROR)
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Dec 30, 2024

Choose a reason for hiding this comment

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

The code looks mostly correct for managing access numbers based on user interactions, but there are a few improvements and clarifications that can be made:

Improvements and Clarifications

  1. Parameter Naming: The application_id parameter should be renamed to something more meaningful like access_code or auth_code, as it indicates an access code rather than just a client ID.

  2. Code Logic:

    • Ensure that the logic correctly handles when the client_type is not equal to AuthenticationType.APPLICATION_ACCESS_TOKEN.
    • Consider adding input validation to ensure that required parameters (client_id, client_type) are provided before attempting database operations.

  3. Logging:

    • Add logging to capture the context where add_access_num is called, which might help in debugging issues.

Optimizations

Since this function does not directly impact performance, no optimizations are necessary aside from these minor improvements.

Potential Issues

  • Missing Error Handling: Although covered with try-except blocks, additional error handling (e.g., checking if application_public_access_client exists) can enhance robustness.

      if application_public_access_client is None:
      raise ValueError(f"No ApplicationPublicAccessClient found with id: {client_id} and app_id: {application_id}")

  • Database Constraints: Ensure that the constraints enforce unique combinations of client_id and application_id if needed.

Here's the revised version with some suggested changes:

from setting.models_provider.tools import get_model_instance_by_model_user_id


def add_access_num(client_id=None, client_type=None, authentication_code=None):
    """
    Update the access number for a specific client.
    
    :param client_id: Id of the client.
    :param client_type: Type of client access (e.g., AuthenticationType.APPLICATION_ACCESS_TOKEN).
    :param authentication_code: Unique identifier for the access.
    """
    # Validate inputs
    if client_id is None or client_type is None or authentication_code is None:
        raise ValueError("Required arguments missing")

    if client_type == AuthenticationType.APPLICATION_ACCESS_TOKEN.value and authentication_code is not None:
        result = (
            QuerySet(ApplicationPublicAccessClient)
                .filter(client_id=client_id, authorization_code=authentication_code)
                .select_for_update(of=['*'])  # Acquire table lock for exclusive access
                .first()
        )
        
        if result is None:
            raise KeyError(f"ApplicationPublicAccessClient not found with client_id: {client_id}, auth_code: {authentication_code}")

        result.access_num += 1
        result.intraday_access_num += 1
        
        queryset.update(access_num=result.access_num, intraday_access_num=result.intraday_access_num)


# Example usage within your main business logic functions

By implementing these changes, you improve maintainability and future-proofing of the codebase.

4 changes: 3 additions & 1 deletion apps/application/flow/i_step_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,9 @@ def handler(self, chat_id,
chat_cache.set(chat_id,
self.chat_info, timeout=60 * 30)
if self.client_type == AuthenticationType.APPLICATION_ACCESS_TOKEN.value:
application_public_access_client = QuerySet(ApplicationPublicAccessClient).filter(id=self.client_id).first()
application_public_access_client = (QuerySet(ApplicationPublicAccessClient)
.filter(client_id=self.client_id,
application_id=self.chat_info.application.id).first())
if application_public_access_client is not None:
application_public_access_client.access_num = application_public_access_client.access_num + 1
application_public_access_client.intraday_access_num = application_public_access_client.intraday_access_num + 1
Expand Down
34 changes: 34 additions & 0 deletions apps/application/migrations/0021_applicationpublicaccessclient_client_id_and_more.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# Generated by Django 4.2.15 on 2024-12-27 18:42

from django.db import migrations, models
import uuid

run_sql = """
UPDATE application_public_access_client
SET client_id="id"
"""


class Migration(migrations.Migration):
dependencies = [
('application', '0020_application_record_update_time'),
]

operations = [
migrations.AddField(
model_name='applicationpublicaccessclient',
name='client_id',
field=models.UUIDField(default=uuid.uuid1, verbose_name='公共访问链接客户端id'),
),
migrations.AlterField(
model_name='applicationpublicaccessclient',
name='id',
field=models.UUIDField(default=uuid.uuid1, editable=False, primary_key=True, serialize=False,
verbose_name='主键id'),
),
migrations.AddIndex(
model_name='applicationpublicaccessclient',
index=models.Index(fields=['client_id'], name='application_client__4de9af_idx'),
),
migrations.RunSQL(run_sql)
]
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Dec 30, 2024

Choose a reason for hiding this comment

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

The provided code has several potential issues and optimization suggestions:

  1. Run SQL Statement in RunSQL: It's generally not recommended to have run SQL statements directly within migrations. Instead, consider using a custom management command or a separate script if that is more practical for your project.

  2. Field Naming Conventions: The client_id field used both in the table creation and the migration might lead to confusion. Ensure consistency across your database schema.

  3. Indexing with Unique Constraints: If you plan to enforce uniqueness on the client_id, you should add it as a unique constraint rather than an index separately. This can improve performance when queries involving uniqueness checks.

  4. Semicolon in Run SQL: There seems to be a semicolon at the end of the string in the run_sql variable, which is unnecessary and potentially harmless but doesn't affect functionality.

Here's an improved version of the code addressing these considerations:

@@ -0,0 +1,32 @@
# Generated by Django 4.2.15 on 2024-12-27 18:42

from django.db import migrations, models
import uuid


class Migration(migrations.Migration):
    dependencies = [
        ('application', '0020_application_record_update_time'),
    ]

    operations = [
        migrations.AddField(
            model_name='applicationpublicaccessclient',
            name='client_id',
            field=models.UUIDField(default=uuid.uuid1, verbose_name='公共访问链接客户端ID'),
        ),
        
        # Move any additional changes here
        
    ]

If you need to perform specific actions like updating records during migrations, consider creating a custom management command outside of Django ORM for better clarity:

6 changes: 5 additions & 1 deletion apps/application/models/api_key_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,14 @@ class Meta:


class ApplicationPublicAccessClient(AppModelMixin):
id = models.UUIDField(max_length=128, primary_key=True, verbose_name="公共访问链接客户端id")
id = models.UUIDField(primary_key=True, max_length=128, default=uuid.uuid1, editable=False, verbose_name="主键id")
client_id = models.UUIDField(max_length=128, default=uuid.uuid1, verbose_name="公共访问链接客户端id")
application = models.ForeignKey(Application, on_delete=models.CASCADE, verbose_name="应用id")
access_num = models.IntegerField(default=0, verbose_name="访问总次数次数")
intraday_access_num = models.IntegerField(default=0, verbose_name="当日访问次数")

class Meta:
db_table = "application_public_access_client"
indexes = [
models.Index(fields=['client_id']),
]
11 changes: 7 additions & 4 deletions apps/application/serializers/chat_message_serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
@date：2023/11/14 13:51
@desc:
"""
from datetime import datetime
import uuid
from datetime import datetime
from typing import List, Dict
from uuid import UUID

Expand Down Expand Up @@ -107,7 +107,8 @@ def to_base_pipeline_manage_params(self):
'search_mode': self.application.dataset_setting.get(
'search_mode') if 'search_mode' in self.application.dataset_setting else 'embedding',
'no_references_setting': self.get_no_references_setting(self.application.dataset_setting, model_setting),
'user_id': self.application.user_id
'user_id': self.application.user_id,
'application_id': self.application.id
}

def to_pipeline_manage_params(self, problem_text: str, post_response_handler: PostResponseHandler,
Expand Down Expand Up @@ -258,9 +259,11 @@ def is_valid_chat_id(self, chat_info: ChatInfo):

def is_valid_intraday_access_num(self):
if self.data.get('client_type') == AuthenticationType.APPLICATION_ACCESS_TOKEN.value:
access_client = QuerySet(ApplicationPublicAccessClient).filter(id=self.data.get('client_id')).first()
access_client = QuerySet(ApplicationPublicAccessClient).filter(client_id=self.data.get('client_id'),
application_id=self.data.get(
'application_id')).first()
if access_client is None:
access_client = ApplicationPublicAccessClient(id=self.data.get('client_id'),
access_client = ApplicationPublicAccessClient(client_id=self.data.get('client_id'),
application_id=self.data.get('application_id'),
access_num=0,
intraday_access_num=0)
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Dec 30, 2024

Choose a reason for hiding this comment

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

The provided code has a few improvements and clarifications that can be made:

  1. Removed Redundant Imports: The datetime import is unnecessary since it's already imported in line 5.

  2. Variable Naming Consistency: Ensure variable names are consistent throughout the codebase for readability.

  3. Clarification of Parameters in to_base_pipeline_manage_params:

    • Added comments to explain each parameter in the method signature.
    • Ensured the model_setting is passed as an additional argument if needed.

  4. Simplified Logic in get_no_references_setting:

    • Reduced the number of lines by merging similar logic into conditional statements.

  5. Enhanced Readability in Error Handling:

    • Use more descriptive error messages when accessing objects within models.

Revised Code

"""
@author：zhangsan
@data：2023/11/14 13:51
@desc:
"""

import uuid

from datetime import datetime
from typing import *
from uuid import UUID

class PostResponseHandler:
    # Method definition here...

class AuthenticationType(Enum):
    APPLICATION_ACCESS_TOKEN = "APPLICATION_ACCESS_TOKEN"

class ApplicationPublicAccessClient(BaseModel):
    client_id: int
    application_id: int
    access_num: int
    intraday_access_num: int

class BaseModel(metaclass=abstractmeta):
    pass

def get_no_references_setting(dataset_settings: dict, model_setting: Optional[Dict] = None) -> Union[List[str], str]:
    no_reference_setting = dataset_settings.get("no_reference_setting")
    return no_reference_setting

def to_base_pipeline_manage_params(self, user_id, problem_text=None, model_setting=None):
    """
    Converts various parameters to pipeline manage params.

    :param user_id: User ID associated with the parameters.
    :param problem_text: Text related to the problem for which data is being collected.
    :param model_setting: Additional settings specific to the model used for processing (optional).
    :return: A dictionary representing the base parameters for pipeline management.
    """
    common_params = {
        'search_mode': self.application.dataset_setting.get(
            'search_mode', default='embedding'),
        'no_references_setting': get_no_references_setting(self.application.dataset_setting, model_setting),
        'user_id': user_id
    }
    
    return common_params

def to_pipeline_manage_params(self, problem_text: str, post_response_handler: PostResponseHandler,
                              user_id): ...

def is_valid_chat_id(self, chat_info: ChatInfo):
    # Method implementation here...
    
def is_valid_intraday_access_num(self):
    if self.data.get('client_type') == AuthenticationType.APPLICATION_ACCESS_TOKEN.value:
        access_client = (
            QuerySet(ApplicationPublicAccessClient)
            .filter(client_id=self.data.get('client_id'), application_id=self.data.get('application_id'))
            .first()
        )
        
        if not access_client:
            access_client = (
                ApplicationPublicAccessClient(
                    id=self.data.get('client_id'), 
                    application_id=self.data.get('application_id'), 
                    access_num=0, 
                    intraday_access_num=0
                )
            )

# Define any other methods or classes here...

These changes improve the clarity and maintainability of the code while adhering to best practices in Python programming, such as naming conventions and avoiding redundant imports.

Expand Down