From 486ddfa6fc6168e3b0b417651a952bdac1bee868 Mon Sep 17 00:00:00 2001 From: Thomas Kaul <4159106+dtslvr@users.noreply.github.com> Date: Mon, 31 Aug 2026 19:32:02 +0200 Subject: [PATCH] Add MCP tool to import activities --- .../app/endpoints/mcp/mcp.controller.spec.ts | 80 ++++++++++++++++--- .../src/app/endpoints/mcp/mcp.controller.ts | 27 ++++--- apps/api/src/app/import/import.service.ts | 39 +++++---- .../data-provider/data-provider.service.ts | 13 +-- libs/common/src/lib/helper.spec.ts | 10 +++ libs/common/src/lib/helper.ts | 2 +- 6 files changed, 128 insertions(+), 43 deletions(-) diff --git a/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts b/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts index 3cc1d35d46..b07441e73e 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts @@ -12,7 +12,7 @@ import type { UserWithSettings } from '@ghostfolio/common/types'; -import { HttpException } from '@nestjs/common'; +import { HttpException, Logger } from '@nestjs/common'; import { RpcException } from '@nestjs/microservices'; import { DataSource, Type as ActivityType } from '@prisma/client'; import { getReasonPhrase, StatusCodes } from 'http-status-codes'; @@ -50,6 +50,7 @@ function createActivity(overrides: Record = {}) { describe('GhostfolioMcpController', () => { const impersonation = { userId: 'user-id' } as ImpersonationContext; + let configuration: Record; let configurationService: ConfigurationService; let controller: GhostfolioMcpController; let importService: ImportService; @@ -62,8 +63,15 @@ describe('GhostfolioMcpController', () => { } beforeEach(() => { + configuration = { + DATA_SOURCES_GHOSTFOLIO_DATA_PROVIDER: [], + ENABLE_FEATURE_SUBSCRIPTION: false + }; + configurationService = { - get: jest.fn().mockReturnValue([]) + get: jest.fn().mockImplementation((key: string) => { + return configuration[key]; + }) } as unknown as ConfigurationService; importService = { @@ -81,6 +89,10 @@ describe('GhostfolioMcpController', () => { ); }); + afterEach(() => { + jest.restoreAllMocks(); + }); + describe('Import activities', () => { it('Requires the scope to create an activity', () => { expect( @@ -127,9 +139,8 @@ describe('GhostfolioMcpController', () => { it('Resolves the mask of the data source of the Ghostfolio data provider', async () => { setupUser([permissions.createActivity]); - jest - .spyOn(configurationService, 'get') - .mockReturnValue([DataSource.YAHOO]); + configuration.DATA_SOURCES_GHOSTFOLIO_DATA_PROVIDER = [DataSource.YAHOO]; + configuration.ENABLE_FEATURE_SUBSCRIPTION = true; await controller.importActivities(impersonation, { activities: [createActivity({ dataSource: DataSource.GHOSTFOLIO })] @@ -144,6 +155,25 @@ describe('GhostfolioMcpController', () => { ); }); + it('Keeps the data source if the subscription is not enabled', async () => { + setupUser([permissions.createActivity]); + + configuration.DATA_SOURCES_GHOSTFOLIO_DATA_PROVIDER = [DataSource.YAHOO]; + configuration.ENABLE_FEATURE_SUBSCRIPTION = false; + + await controller.importActivities(impersonation, { + activities: [createActivity({ dataSource: DataSource.GHOSTFOLIO })] + }); + + expect(importService.import).toHaveBeenCalledWith( + expect.objectContaining({ + activitiesDto: [ + expect.objectContaining({ dataSource: DataSource.GHOSTFOLIO }) + ] + }) + ); + }); + it('Passes on the message of a validation only', async () => { setupUser([permissions.createActivity]); @@ -162,14 +192,18 @@ describe('GhostfolioMcpController', () => { ); }); - it('Hides the message of an unexpected error', async () => { + it('Hides the message of an unexpected error and writes it to the log', async () => { setupUser([permissions.createActivity]); - jest - .spyOn(importService, 'import') - .mockRejectedValue( - new Error('Unique constraint failed on the fields: (dataSource)') - ); + const error = new Error( + 'Unique constraint failed on the fields: (dataSource)' + ); + + const logError = jest + .spyOn(Logger.prototype, 'error') + .mockImplementation(); + + jest.spyOn(importService, 'import').mockRejectedValue(error); await expect( controller.importActivities(impersonation, { @@ -178,6 +212,30 @@ describe('GhostfolioMcpController', () => { ).rejects.toThrow( new RpcException(getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR)) ); + + expect(logError).toHaveBeenCalledWith(error); + }); + + it('Does not write the message of a validation to the log', async () => { + setupUser([permissions.createActivity]); + + const logError = jest + .spyOn(Logger.prototype, 'error') + .mockImplementation(); + + jest + .spyOn(importService, 'import') + .mockRejectedValue( + new ImportValidationError('activities.0.accountId ("X") is not valid') + ); + + await expect( + controller.importActivities(impersonation, { + activities: [createActivity({ accountId: 'X' })] + }) + ).rejects.toThrow(RpcException); + + expect(logError).not.toHaveBeenCalled(); }); }); diff --git a/apps/api/src/app/endpoints/mcp/mcp.controller.ts b/apps/api/src/app/endpoints/mcp/mcp.controller.ts index df3cbd7141..baf68659b0 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.controller.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.controller.ts @@ -25,7 +25,7 @@ import { hasPermission, permissions } from '@ghostfolio/common/permissions'; import { scopes } from '@ghostfolio/common/scopes'; import type { ImpersonationContext } from '@ghostfolio/common/types'; -import { HttpException, UseFilters } from '@nestjs/common'; +import { HttpException, Logger, UseFilters } from '@nestjs/common'; import { Payload, RpcException } from '@nestjs/microservices'; import { AssetClass, DataSource, Type as ActivityType } from '@prisma/client'; import { McpController, Tool } from '@rekog/mcp-nest'; @@ -142,6 +142,8 @@ export const IMPORT_ACTIVITIES_PARAMETERS = z.object({ @McpController() @UseFilters(McpToolExceptionFilter) export class GhostfolioMcpController { + private readonly logger = new Logger(GhostfolioMcpController.name); + public constructor( private readonly aiService: AiService, private readonly apiService: ApiService, @@ -278,7 +280,7 @@ export class GhostfolioMcpController { readOnlyHint: false, title: 'Import activities' }, - description: `Imports activities into the portfolio and gives the number of the imported activities and the number of the skipped activities. An activity is skipped if an equal activity is in the portfolio already, hence send each activity one time only: two equal activities of the same call are both imported. The access needs the permission "Restricted view and manage". At most ${MCP_MAX_ACTIVITIES} activities are imported per call. An error does not remove the activities of the same call which are imported already, hence get the activities after an error before you import them again.`, + description: `Imports activities into the portfolio and gives the number of the imported activities and the number of the skipped activities. An activity is skipped if an equal activity is in the portfolio already, hence send each activity one time only: two equal activities of the same call are both imported. The access needs the permission "Restricted view and manage". At most ${MCP_MAX_ACTIVITIES} activities are imported per call, while the instance can have a lower limit, which an error names. An error does not remove the activities of the same call which are imported already, hence get the activities after an error before you import them again.`, name: 'import-activities', parameters: IMPORT_ACTIVITIES_PARAMETERS }) @@ -296,8 +298,10 @@ export class GhostfolioMcpController { } const ghostfolioDataSources = this.configurationService.get( - 'DATA_SOURCES_GHOSTFOLIO_DATA_PROVIDER' - ); + 'ENABLE_FEATURE_SUBSCRIPTION' + ) + ? this.configurationService.get('DATA_SOURCES_GHOSTFOLIO_DATA_PROVIDER') + : []; const activitiesDto = activities.map((activity) => { return { @@ -322,12 +326,17 @@ export class GhostfolioMcpController { }); } catch (error) { // The message of a validation names the activity which is not valid and - // is written for the caller, hence it is passed on. Every other error - // can carry internals of the application. + // is written for the caller, hence it is passed on + if (error instanceof ImportValidationError) { + throw new RpcException(error.message); + } + + // Every other message can carry internals of the application, hence it + // is written to the log and the reason phrase is passed on instead + this.logger.error(error); + throw new RpcException( - error instanceof ImportValidationError - ? error.message - : getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR) + getReasonPhrase(StatusCodes.INTERNAL_SERVER_ERROR) ); } diff --git a/apps/api/src/app/import/import.service.ts b/apps/api/src/app/import/import.service.ts index 882557e01a..5f202f4ac4 100644 --- a/apps/api/src/app/import/import.service.ts +++ b/apps/api/src/app/import/import.service.ts @@ -43,7 +43,7 @@ import { } from '@ghostfolio/common/types'; import { Injectable } from '@nestjs/common'; -import { Account, DataSource, Prisma, SymbolProfile } from '@prisma/client'; +import { Account, DataSource, Prisma } from '@prisma/client'; import { Big } from 'big.js'; import { isISIN } from 'class-validator'; import { isSameSecond, parseISO } from 'date-fns'; @@ -741,22 +741,12 @@ export class ImportService { } } - let assetProfiles: { - [assetProfileIdentifier: string]: Partial; - }; - - try { - assetProfiles = await this.dataProviderService.validateActivities({ - activitiesDto, - assetProfilesWithMarketDataDto, - maxActivitiesToImport, - subscription: user.subscription - }); - } catch (error) { - // The messages of the validation name the activity which is not valid - // and are written for the caller - throw new ImportValidationError(error.message); - } + const assetProfiles = await this.dataProviderService.validateActivities({ + activitiesDto, + assetProfilesWithMarketDataDto, + maxActivitiesToImport, + subscription: user.subscription + }); const activitiesExtendedWithErrors = await this.extendActivitiesWithErrors({ activitiesDto, @@ -787,6 +777,21 @@ export class ImportService { }); } + // Validate the accounts before any activity is created, since an account + // which does not belong to the user is dropped without a notice otherwise + for (const [index, { accountId }] of activitiesDto.entries()) { + if ( + accountId && + !accounts.some(({ id }) => { + return id === accountId; + }) + ) { + throw new ImportValidationError( + `activities.${index}.accountId ("${accountId}") is not valid` + ); + } + } + const tags = (await this.tagService.getTagsForUser(user.id)).map( ({ id, name }) => { return { id, name }; diff --git a/apps/api/src/services/data-provider/data-provider.service.ts b/apps/api/src/services/data-provider/data-provider.service.ts index 0d0924f80d..9366574ac4 100644 --- a/apps/api/src/services/data-provider/data-provider.service.ts +++ b/apps/api/src/services/data-provider/data-provider.service.ts @@ -1,3 +1,4 @@ +import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import-validation.error'; import { ImportDataDto } from '@ghostfolio/api/app/import/import-data.dto'; import { RedisCacheService } from '@ghostfolio/api/app/redis-cache/redis-cache.service'; import { getMaskedGhostfolioDataSource } from '@ghostfolio/api/helper/data-source.helper'; @@ -226,7 +227,9 @@ export class DataProviderService implements OnModuleInit { subscription: UserWithSettings['subscription']; }) { if (activitiesDto?.length > maxActivitiesToImport) { - throw new Error(`Too many activities (${maxActivitiesToImport} at most)`); + throw new ImportValidationError( + `Too many activities (${maxActivitiesToImport} at most)` + ); } const assetProfiles: { @@ -250,7 +253,7 @@ export class DataProviderService implements OnModuleInit { }); if (!dataSources.includes(dataSource)) { - throw new Error( + throw new ImportValidationError( `${activityPath}.dataSource ("${dataSource}") is not valid` ); } @@ -259,7 +262,7 @@ export class DataProviderService implements OnModuleInit { dataSource !== DataSource.MANUAL && isValidCustomAssetProfileSymbol(symbol) ) { - throw new Error( + throw new ImportValidationError( `${activityPath}.symbol ("${symbol}") is not valid for the data source ("${maskedDataSource}")` ); } @@ -271,7 +274,7 @@ export class DataProviderService implements OnModuleInit { const dataProvider = this.getDataProvider(DataSource[dataSource]); if (dataProvider.getDataProviderInfo().isPremium) { - throw new Error( + throw new ImportValidationError( `${activityPath}.dataSource ("${maskedDataSource}") requires Ghostfolio Premium` ); } @@ -328,7 +331,7 @@ export class DataProviderService implements OnModuleInit { } catch {} if (!assetProfile?.name) { - throw new Error( + throw new ImportValidationError( `${activityPath}.symbol ("${symbol}") cannot be resolved by the data source ("${maskedDataSource}")` ); } diff --git a/libs/common/src/lib/helper.spec.ts b/libs/common/src/lib/helper.spec.ts index 78c42a779a..727672a23a 100644 --- a/libs/common/src/lib/helper.spec.ts +++ b/libs/common/src/lib/helper.spec.ts @@ -440,6 +440,16 @@ describe('Helper', () => { expect(isValidDateAfter1970('1970-01-01T00:00:00.000Z')).toEqual(false); }); + // A date without a time is read in UTC, hence the result is the same in + // every time zone of the server + it('Date of 1970 without a time', () => { + expect(isValidDateAfter1970('1970-01-01')).toEqual(false); + }); + + it('Date after 1970 without a time', () => { + expect(isValidDateAfter1970('1970-01-02')).toEqual(true); + }); + it('Date with an expanded year', () => { expect(isValidDateAfter1970('+010000-01-01')).toEqual(true); }); diff --git a/libs/common/src/lib/helper.ts b/libs/common/src/lib/helper.ts index a813e55c62..48ecc5ac8c 100644 --- a/libs/common/src/lib/helper.ts +++ b/libs/common/src/lib/helper.ts @@ -686,7 +686,7 @@ export function isValidCustomAssetProfileSymbol(aSymbol: string) { } export function isValidDateAfter1970(aDate: string) { - const date = parseISO(aDate); + const date = parseISO(aDate, { in: utc }); return isValid(date) && isAfter(date, new Date(0)); }