Skip to content

refactor: replace OrgResolver with LdxSyncService and cache-based approach #1129

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

Jump to bottom
Open
nick-y-snyk wants to merge 11 commits into
base: main
Choose a base branch
from
Open

refactor: replace OrgResolver with LdxSyncService and cache-based approach #1129

nick-y-snyk wants to merge 11 commits into from

Conversation

@nick-y-snyk
Copy link
Contributor

@nick-y-snyk nick-y-snyk commented Jan 26, 2026 •
edited
Loading

Summary

  • Replace OrgResolver with LdxSyncService for organization resolution
  • Implement cache-based approach for improved performance

Test plan

  • Verify organization resolution works correctly
  • Confirm caching behavior is functioning as expected
  • Run existing tests to ensure no regressions

@nick-y-snyk
refactor: replace OrgResolver with LdxSyncService and cache-based app…
611e9f8 
…roach

- Introduced LdxSyncService interface for LDX-Sync configuration management
- Added in-memory caching of LDX-Sync results at Config level
- Implemented parallel fetching of LDX-Sync config for workspace folders
- Cache refreshes on login and workspace folder changes
- Replaced OrgResolver interface with direct cache-based lookups
- Removed deprecated GetBestOrgFromLdxSync function
- Updated all tests to use GetOrgFromCachedLdxSync directly
- Changed successful fetch logging from Info to Debug level
- Added ABOUTME comments to new ldx_sync_service.go file
- Documented 4 skipped integration tests in SKIPPED_TESTS_IDE-1636.md

This refactor improves performance by avoiding repeated API calls and
provides a cleaner architecture with better separation of concerns.
@nick-y-snyk nick-y-snyk requested review from a team as code owners January 26, 2026 11:26
@snyk-io
Copy link

snyk-io bot commented Jan 26, 2026 •
edited
Loading

✅ Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
✅ Open Source Security 0 0 0 0 0 issues
✅ Licenses 0 0 0 0 0 issues
✅ Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@gemini-code-assist
Copy link
Contributor

gemini-code-assist bot commented Jan 26, 2026

Summary of Changes

Hello @nick-y-snyk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on enhancing the developer experience by re-enabling a local module replacement for the go-application-framework, which is crucial for local development and testing. Concurrently, it introduces a significant refactoring of how LDX-Sync organization configurations are managed. A new dedicated service and an in-memory caching mechanism are now responsible for fetching and storing these configurations, leading to more efficient and testable organization resolution across the application. This ensures that organization data is consistently available and updated at critical points in the application's lifecycle.

Highlights

  • Local Go Application Framework Replacement: The local replacement directive for go-application-framework in go.mod has been re-enabled, facilitating local development and testing with the framework.
  • LDX-Sync Configuration Refactoring: The mechanism for resolving LDX-Sync organization configurations has been refactored. The previous OrgResolver interface has been removed and replaced with a new LdxSyncService.
  • In-Memory Caching for LDX-Sync Results: An in-memory caching system has been implemented within the Config object to store LDX-Sync results, improving efficiency and reducing redundant API calls. Organization resolution now primarily uses this cache, with a fallback to the global organization ID if no cached entry is found.
  • Lifecycle Integration of LDX-Sync Refresh: The RefreshConfigFromLdxSync method is now invoked at key application lifecycle events, including server initialization, workspace folder changes, and successful user login, to ensure the LDX-Sync cache is up-to-date.
  • Documentation of Skipped Tests: A new markdown file (SKIPPED_TESTS_IDE-1636.md) has been added to document four integration tests related to LDX-Sync caching that are currently skipped due to infrastructure requirements, outlining their purpose and future recommendations.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@nick-y-snyk nick-y-snyk changed the title chore: enable local go-application-framework replacement refactor: replace OrgResolver with LdxSyncService and cache-based approach Jan 26, 2026
gemini-code-assist[bot]
gemini-code-assist bot reviewed Jan 26, 2026
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new LdxSyncService to manage and cache LDX-Sync configuration results for workspace folders. This refactoring improves how organization-related settings are handled, moving from a direct OrgResolver to a caching mechanism. The changes involve updating dependency injection, modifying relevant handlers to utilize the new service, and updating tests to reflect these architectural shifts. A new markdown file has also been added to document skipped integration tests, which is a good practice for future development.

@nick-y-snyk
refactor: clean up folder_handler tests and remove network mocking
e9c35f2 
- Remove unnecessary network mocking in tests (mock at service level instead)
- Use generated MockLdxSyncService instead of manual mocks
- Add ResolveOrg method to LdxSyncService interface
- Fix fallback tests by properly initializing engine mock
- Delete SKIPPED_TESTS file as all tests now pass
ShawkyZ
ShawkyZ reviewed Jan 26, 2026
View reviewed changes
application/config/config.go
func (c *Config) InitLdxSyncCache() {
c.ldxSyncCacheMutex.Lock()
defer c.ldxSyncCacheMutex.Unlock()
c.ldxSyncCache = make(map[types.FilePath]*ldx_sync_config.LdxSyncConfigResult)
Copy link
Collaborator

@ShawkyZ ShawkyZ Jan 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

why build a cache here and not use GAF config cache ? or maybe I'm missing something?

Copy link
Contributor Author

@nick-y-snyk nick-y-snyk Jan 27, 2026

Choose a reason for hiding this comment

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

we want to hold ldxsync configs inmemory in ls, later it will be structured better; it's just the initial approach

Copy link
Collaborator

@ShawkyZ ShawkyZ Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

GAF caches config values in mem already

nick-y-snyk
nick-y-snyk commented Jan 27, 2026
View reviewed changes
domain/ide/command/folder_handler.go Outdated
gafConfig := engine.GetConfiguration()
// GetOrgFromCachedLdxSync retrieves the organization from the cached LDX-Sync result
// Falls back to global organization if no cache entry exists
func GetOrgFromCachedLdxSync(c *config.Config, folderPath types.FilePath, ldxSyncService LdxSyncService) (ldx_sync_config.Organization, error) {
Copy link
Contributor Author

@nick-y-snyk nick-y-snyk Jan 27, 2026

Choose a reason for hiding this comment

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

move this to a service

@nick-y-snyk
Copy link
Contributor Author

nick-y-snyk commented Jan 27, 2026

Refactoring Summary: LdxSyncService as Complete Facade

Changes Made

  1. Moved GetOrgFromCachedLdxSync into LdxSyncService

    • Renamed method to ResolveOrg(c *config.Config, folderPath types.FilePath)
    • Changed signature from accepting LdxSyncConfigResult to accepting folderPath
    • Service now handles cache lookup internally, making it a complete facade for all LDX-Sync operations

  2. Eliminated Mock Complexity

    • Replaced setupMockLdxSyncService() with setupLdxSyncService() that returns the real implementation
    • No mocking needed since ResolveOrg() just reads from cache (which tests populate) and GAF config
    • Simpler, more maintainable tests

  3. Removed Redundant Tests

    • Deleted Test_SetAutoBestOrgFromLdxSync_DefaultOrg
    • Deleted Test_SetAutoBestOrgFromLdxSync_NonDefaultOrg
    • Deleted setupOrgResolverTest helper

Why Remove These Tests?

These tests were testing GAF's ResolveOrgFromUserConfig behavior (whether it correctly extracts org from a cached result with PreferredByAlgorithm=true). This logic is already tested in GAF at:

go-application-framework/pkg/apiclients/ldx_sync_config/resolver_test.go

Our LdxSyncService.ResolveOrg() is now just a facade that:

  1. Gets cached result from config
  2. Delegates to GAF's ResolveOrgFromUserConfig (tested by GAF)
  3. Falls back to global org if no cache

We should test our responsibilities (cache lookup, fallback logic), not re-test GAF's resolver logic.

Tests That Remain

The remaining tests cover our actual facade responsibilities:

  • ✅ Test_GetOrgFromCachedLdxSync_WithCache - cache lookup works
  • ✅ Test_SetAutoBestOrgFromLdxSync_ErrorHandling - fallback to global org when cache empty
  • ✅ Test_SetAutoBestOrgFromLdxSync_FallbackToGafConfig - fallback behavior
  • ✅ Test_GetOrgFromCachedLdxSync_WithoutCache_FallbackToGlobal - fallback behavior
  • ✅ Integration tests via Test_sendFolderConfigs_* - end-to-end behavior

Benefits

  1. Single Entry Point: LdxSyncService is now the complete facade for all LDX-Sync operations
  2. No Mocking: Tests use real implementation, making them more reliable
  3. Clearer Boundaries: We test our code, GAF tests theirs
  4. Better Warning: Added warning log when falling back to global org (ldx_sync_service.go:117)

@nick-y-snyk
test: add comprehensive tests for LdxSyncService with API mocking
c669687 
Refactored LdxSyncService to use dependency injection, enabling full mocking of external LDX-Sync API calls in tests. Added 16 test cases covering RefreshConfigFromLdxSync and ResolveOrg including success paths, error handling, caching, parallel execution, and boundary conditions.
@nick-y-snyk
fix: return error from ResolveOrg when LDX-Sync cache is empty
792afb7 
LdxSyncService.ResolveOrg previously fell back to the global organization
when no cache entry existed for a folder. This could lead to unexpected
behavior where folders would use a different org than intended by LDX-Sync.

Changes ResolveOrg to return an error instead of falling back, ensuring
that callers explicitly handle the case where organization cannot be
determined from the LDX-Sync cache. This makes the behavior more
predictable and easier to reason about.

Also fixes Test_loginCommand_StartsAuthentication timeout by adding
missing mock expectation for ResolveOrg method call during initialized
handler execution.
@nick-y-snyk
test: use explicit mocks for LdxSyncService in unit tests
9cdec84 
Replace cache pre-population with mock LdxSyncService for better test
isolation and control. Tests that specifically verify cache logic still
use real service with cache, but general behavior tests now use mocks
to avoid implicit dependencies on cache state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShawkyZ ShawkyZ ShawkyZ left review comments

@andrewrobinsonhodges-snyk andrewrobinsonhodges-snyk andrewrobinsonhodges-snyk approved these changes

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nick-y-snyk @ShawkyZ @andrewrobinsonhodges-snyk