Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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: 14 additions & 2 deletions backend/apps/github/api/internal/nodes/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,10 @@
"avatar_url",
"bio",
"company",
"contributions_count",
"email",
"followers_count",
"following_count",
"id",
"is_owasp_staff",
"location",
"login",
"name",
Expand All @@ -28,6 +26,13 @@
class UserNode:
"""GitHub user node."""

@strawberry.field
def contributions_count(self) -> int:
"""Resolve contributions count."""
if hasattr(self, "owasp_profile") and self.owasp_profile.contributions_count:
return self.owasp_profile.contributions_count
return self.contributions_count
Comment on lines +29 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

cat -n backend/apps/github/api/internal/nodes/user.py | head -100

Repository: OWASP/Nest

Length of output: 164


🌐 Web query:

strawberry field decorator method shadowing infinite recursion

💡 Result:

Problem: if you define a Strawberry resolver method with the same Python name as a dataclass field and that method reads the attribute via self., Python will call the method again (not the stored field), causing infinite recursion.

Why it happens (short): Strawberry lets you attach resolvers as methods. A method named X and a field X collide at attribute lookup: self.X resolves to the method, so returning self.X (or accessing the attribute in a way that triggers the method) calls the method again. See Strawberry resolvers docs. [1][2]

Minimal example that causes recursion:

@strawberry.type
class User:
    name: str

    @strawberry.field
    def name(self) -> str:
        return self.name    # -> calls the same method again => infinite recursion

Fixes (pick one):

  • Rename the resolver method so it doesn't shadow the attribute, and expose a GraphQL name if needed:

    @strawberry.field(name="name")
    def resolve_name(self) -> str:
    return self._name_or_other_source

    (strawberry.field supports a name parameter). [2]

  • Use a separate resolver function and attach it to the field, avoiding method shadowing:

    def get_name(root) -> str:
    return root.name # root is the underlying data object

    class User:
    name: str = strawberry.field(resolver=get_name)

    (see docs for function resolvers). [1][3]

  • If the field value is stored on a different attribute (e.g., _name), read that directly in the resolver to avoid self.name recursion.

References:

  • Strawberry resolvers and method-based resolvers. [1]
  • strawberry.field API (name parameter & decorator usage). [2]
  • Examples of query/field resolver patterns. [3]

Would you like a corrected code example for your specific schema?


Infinite recursion in resolver: method shadows model field.

The contributions_count method on line 34 returns self.contributions_count, which recursively calls the same method instead of accessing the underlying model field. This is a known pattern collision in Strawberry GraphQL when a resolver method name matches a dataclass field.

Use a cleaner fix: rename the method and expose the GraphQL field name:

     @strawberry.field
-    def contributions_count(self) -> int:
+    def resolve_contributions_count(self) -> int:
         """Resolve contributions count."""
         if hasattr(self, "owasp_profile") and self.owasp_profile.contributions_count:
             return self.owasp_profile.contributions_count
-        return self.contributions_count
+        return super().__getattribute__("contributions_count")

Then add name="contributions_count" to the @strawberry.field decorator to preserve the GraphQL schema name, or use a function-based resolver to avoid method shadowing entirely.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In backend/apps/github/api/internal/nodes/user.py around lines 29 to 34, the
Strawberry resolver method contributions_count shadows the model field and ends
up calling itself (infinite recursion); rename the method (e.g.,
_resolve_contributions_count) to avoid the attribute collision and update the
decorator to preserve the GraphQL name by using
@strawberry.field(name="contributions_count") or alternatively convert to a
standalone function-based resolver that reads the underlying model attribute
(e.g., self.owasp_profile.contributions_count or
self._model.contributions_count) and return that value.


@strawberry.field
def badge_count(self) -> int:
"""Resolve badge count."""
Expand Down Expand Up @@ -83,6 +88,13 @@ def is_gsoc_mentor(self) -> bool:
return self.owasp_profile.is_gsoc_mentor
return False

@strawberry.field
def is_owasp_staff(self) -> bool:
"""Resolve if the user is an OWASP staff member."""
if hasattr(self, "owasp_profile"):
return self.owasp_profile.is_owasp_staff
return self.is_owasp_staff
Comment on lines +91 to +96
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Infinite recursion: resolver calls itself.

Same issue as contributions_count: return self.is_owasp_staff on line 96 recursively calls this resolver method.

The pattern used elsewhere in this file (e.g., is_owasp_board_member returning False as default) avoids this issue. Consider:

     @strawberry.field
     def is_owasp_staff(self) -> bool:
         """Resolve if the user is an OWASP staff member."""
         if hasattr(self, "owasp_profile"):
             return self.owasp_profile.is_owasp_staff
-        return self.is_owasp_staff
+        return False

However, this changes backward compatibility behavior. If User.is_owasp_staff field must be preserved as fallback, you'll need to access it differently—either via User.__dict__ access pattern or by keeping is_owasp_staff in the Strawberry type's fields list and using a different resolver name.

🤖 Prompt for AI Agents
In backend/apps/github/api/internal/nodes/user.py around lines 91-96 the
resolver is_owasp_staff returns self.is_owasp_staff which recursively calls the
resolver; change it to not call the resolver itself — either return the same
safe default used elsewhere (e.g., False) or, if you must preserve the original
field value as fallback, read the underlying attribute directly (e.g., via
self.__dict__.get("is_owasp_staff")) or rename the resolver to avoid the field
name collision so you never call self.is_owasp_staff from inside the resolver.


@strawberry.field
def issues_count(self) -> int:
"""Resolve issues count."""
Expand Down
31 changes: 27 additions & 4 deletions backend/apps/github/management/commands/github_update_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from apps.common.models import BATCH_SIZE
from apps.github.models.repository_contributor import RepositoryContributor
from apps.github.models.user import User
from apps.owasp.models.member_profile import MemberProfile

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -45,15 +46,37 @@ def handle(self, *args, **options):
.values("user_id")
.annotate(total_contributions=Sum("contributions_count"))
}
profiles = []
users = []
for idx, user in enumerate(active_users[offset:]):
prefix = f"{idx + offset + 1} of {active_users_count - offset}"
print(f"{prefix:<10} {user.title}")

user.contributions_count = user_contributions.get(user.id, 0)
profile, created = MemberProfile.objects.get_or_create(github_user=user)
if created:
profile.github_user = user
contributions = user_contributions.get(user.id, 0)
profile.contributions_count = contributions
profiles.append(profile)

user.contributions_count = contributions
users.append(user)

if not len(users) % BATCH_SIZE:
User.bulk_save(users, fields=("contributions_count",))
if not len(profiles) % BATCH_SIZE:
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)
User.bulk_save(
users,
fields=("contributions_count",),
)

User.bulk_save(users, fields=("contributions_count",))
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)
User.bulk_save(
users,
fields=("contributions_count",),
)
Comment on lines +65 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Batch logic causes duplicate saves and accumulates memory.

Two issues with the current batching approach:

  1. Duplicate saves: The batch save inside the loop (lines 66-73) saves all accumulated items, not just the current batch. Then the final save (lines 75-82) re-saves everything again.

  2. Memory: Lists grow unbounded; only final batch should be saved at the end.

Apply this diff to fix the batching:

-            if not len(profiles) % BATCH_SIZE:
+            if len(profiles) >= BATCH_SIZE:
                 MemberProfile.bulk_save(
                     profiles,
                     fields=("contributions_count",),
                 )
                 User.bulk_save(
                     users,
                     fields=("contributions_count",),
                 )
+                profiles = []
+                users = []

-        MemberProfile.bulk_save(
-            profiles,
-            fields=("contributions_count",),
-        )
-        User.bulk_save(
-            users,
-            fields=("contributions_count",),
-        )
+        if profiles:
+            MemberProfile.bulk_save(
+                profiles,
+                fields=("contributions_count",),
+            )
+        if users:
+            User.bulk_save(
+                users,
+                fields=("contributions_count",),
+            )

This clears the lists after each batch save to prevent duplicate saves and control memory usage, and only saves remaining items if any exist.

📝 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.

Suggested change
if not len(profiles) % BATCH_SIZE:
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)
User.bulk_save(
users,
fields=("contributions_count",),
)
User.bulk_save(users, fields=("contributions_count",))
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)
User.bulk_save(
users,
fields=("contributions_count",),
)
if len(profiles) >= BATCH_SIZE:
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)
User.bulk_save(
users,
fields=("contributions_count",),
)
profiles = []
users = []
if profiles:
MemberProfile.bulk_save(
profiles,
fields=("contributions_count",),
)
if users:
User.bulk_save(
users,
fields=("contributions_count",),
)

2 changes: 2 additions & 0 deletions backend/apps/github/models/mixins/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,8 @@ def idx_contributions(self):
@property
def idx_contributions_count(self) -> int:
"""Return contributions count for indexing."""
if hasattr(self, "owasp_profile") and self.owasp_profile.contributions_count:
return int(self.owasp_profile.contributions_count)
return self.contributions_count

@property
Expand Down
4 changes: 3 additions & 1 deletion backend/apps/nest/api/internal/nodes/user.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also here

Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,6 @@ class AuthUserNode(strawberry.relay.Node):
@strawberry_django.field
def is_owasp_staff(self) -> bool:
"""Check if the user is an OWASP staff member."""
return self.github_user.is_owasp_staff if self.github_user else False
if hasattr(self.github_user, "owasp_profile"):
return self.github_user.owasp_profile.is_owasp_staff
return self.github_user.is_owasp_staff
12 changes: 11 additions & 1 deletion backend/apps/owasp/admin/member_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ class MemberProfileAdmin(admin.ModelAdmin):
autocomplete_fields = ("github_user",)
list_display = (
"github_user",
"is_owasp_staff",
"has_public_member_page",
"contributions_count",
"owasp_slack_id",
"first_contribution_at",
"is_owasp_board_member",
Expand Down Expand Up @@ -47,7 +50,14 @@ class MemberProfileAdmin(admin.ModelAdmin):
),
(
"Contribution Information",
{"fields": ("first_contribution_at",)},
{
"fields": (
"first_contribution_at",
"is_owasp_staff",
"has_public_member_page",
"contributions_count",
)
},
),
(
"Membership Flags",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@ class HasDashboardAccess(BasePermission):

def has_permission(self, source, info, **kwargs) -> bool:
"""Check if the user has dashboard access."""
return (
(user := info.context.request.user)
and user.is_authenticated
and user.github_user.is_owasp_staff
)
user = info.context.request.user
if not (user and user.is_authenticated and user.github_user):
return False

if hasattr(user.github_user, "owasp_profile"):
return user.github_user.owasp_profile.is_owasp_staff

return user.github_user.is_owasp_staff
10 changes: 9 additions & 1 deletion backend/apps/owasp/api/internal/views/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,15 @@

def has_dashboard_permission(request):
"""Check if user has dashboard access."""
return (user := request.user) and user.is_authenticated and user.github_user.is_owasp_staff
user = request.user
if not (user and user.is_authenticated and hasattr(user, "github_user") and user.github_user):
return False

github_user = user.github_user
if hasattr(github_user, "owasp_profile"):
return github_user.owasp_profile.is_owasp_staff

return github_user.is_owasp_staff


def dashboard_access_required(view_func):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# Generated by Django 5.2.7 on 2025-11-18 17:59

from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("owasp", "0065_memberprofile_linkedin_page_id"),
]

operations = [
migrations.AddField(
model_name="memberprofile",
name="contributions_count",
field=models.PositiveIntegerField(default=0, verbose_name="Contributions count"),
),
migrations.AddField(
model_name="memberprofile",
name="has_public_member_page",
field=models.BooleanField(default=True),
),
migrations.AddField(
model_name="memberprofile",
name="is_owasp_staff",
field=models.BooleanField(
default=False,
help_text="Indicates if the user is OWASP Foundation staff.",
verbose_name="Is OWASP Staff",
),
),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Generated by Django 5.2.7 on 2025-11-18 18:04

from django.db import migrations


def copy_user_data_to_member_profile(apps, _schema_editor):
"""Copy user data to member profile."""
User = apps.get_model("github", "User")
MemberProfile = apps.get_model("owasp", "MemberProfile")
profiles_to_update = []
batch_size = 500
update_fields = [
"has_public_member_page",
"is_owasp_staff",
"contributions_count",
]

for user in User.objects.all().iterator(chunk_size=batch_size):
profile, _ = MemberProfile.objects.get_or_create(github_user=user)
profile.has_public_member_page = user.has_public_member_page
profile.is_owasp_staff = user.is_owasp_staff
profile.contributions_count = user.contributions_count
profiles_to_update.append(profile)

if len(profiles_to_update) >= batch_size:
MemberProfile.objects.bulk_update(
profiles_to_update, update_fields, batch_size=batch_size
)
profiles_to_update = []

if profiles_to_update:
MemberProfile.objects.bulk_update(profiles_to_update, update_fields, batch_size=batch_size)


class Migration(migrations.Migration):
dependencies = [
("owasp", "0066_memberprofile_contributions_count_and_more"),
("github", "0039_remove_commit_commit_repo_created_idx"),
]

operations = [
migrations.RunPython(copy_user_data_to_member_profile, migrations.RunPython.noop),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Generated by Django 5.2.8 on 2025-11-26 14:23

from django.db import migrations, models


class Migration(migrations.Migration):
dependencies = [
("owasp", "0067_memberprofile_backward_compatibility"),
]

operations = [
migrations.AlterField(
model_name="memberprofile",
name="has_public_member_page",
field=models.BooleanField(
default=True,
help_text="Whether the member's profile is publicly visible on the OWASP website",
verbose_name="Has Public Member Page",
),
),
]
21 changes: 20 additions & 1 deletion backend/apps/owasp/models/member_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from django.core.validators import RegexValidator
from django.db import models

from apps.common.models import TimestampedModel
from apps.common.models import BulkSaveModel, TimestampedModel
from apps.github.models.user import User


Expand Down Expand Up @@ -71,6 +71,25 @@ class Meta:
help_text="LinkedIn username or custom URL ID (e.g., 'john-doe-123')",
)

has_public_member_page = models.BooleanField(
default=True,
verbose_name="Has Public Member Page",
help_text="Whether the member's profile is publicly visible on the OWASP website",
)
is_owasp_staff = models.BooleanField(
default=False,
verbose_name="Is OWASP Staff",
help_text="Indicates if the user is OWASP Foundation staff.",
)
contributions_count = models.PositiveIntegerField(
verbose_name="Contributions count", default=0
)

def __str__(self) -> str:
"""Return human-readable representation."""
return f"OWASP member profile for {self.github_user.login}"

@staticmethod
def bulk_save(profiles, fields=None) -> None:
"""Bulk save member profiles."""
BulkSaveModel.bulk_save(MemberProfile, profiles, fields=fields)
18 changes: 7 additions & 11 deletions backend/tests/apps/github/api/internal/queries/user_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,44 +17,40 @@ def mock_user(self):
def test_resolve_user_existing_with_public_member_page(self, mock_user):
"""Test resolving an existing user with has_public_member_page=True."""
with patch("apps.github.models.user.User.objects.filter") as mock_filter:
mock_queryset = mock_filter.return_value
mock_queryset.first.return_value = mock_user
mock_filter.return_value.first.return_value = mock_user

result = UserQuery().user(login="test-user")

assert result == mock_user
mock_filter.assert_called_once_with(has_public_member_page=True, login="test-user")
mock_queryset.first.assert_called_once()
mock_filter.return_value.first.assert_called_once()

def test_resolve_user_not_found_when_has_public_member_page_false(self):
"""Test resolving a user with has_public_member_page=False returns None."""
with patch("apps.github.models.user.User.objects.filter") as mock_filter:
mock_queryset = mock_filter.return_value
mock_queryset.first.return_value = None
mock_filter.return_value.first.return_value = None

result = UserQuery().user(login="test-user")

assert result is None
mock_filter.assert_called_once_with(has_public_member_page=True, login="test-user")
mock_queryset.first.assert_called_once()
mock_filter.return_value.first.assert_called_once()

def test_resolve_user_not_found(self):
"""Test resolving a non-existent user."""
with patch("apps.github.models.user.User.objects.filter") as mock_filter:
mock_queryset = mock_filter.return_value
mock_queryset.first.return_value = None
mock_filter.return_value.first.return_value = None

result = UserQuery().user(login="non-existent")

assert result is None
mock_filter.assert_called_once_with(has_public_member_page=True, login="non-existent")
mock_queryset.first.assert_called_once()
mock_filter.return_value.first.assert_called_once()

def test_resolve_user_filters_by_public_member_page_and_login(self):
"""Test that user query filters by both has_public_member_page and login."""
with patch("apps.github.models.user.User.objects.filter") as mock_filter:
mock_queryset = mock_filter.return_value
mock_queryset.first.return_value = None
mock_filter.return_value.first.return_value = None

UserQuery().user(login="test-user")

Expand Down
Loading