diff --git a/c-sharp-tests/DummyPapiClient.cs b/c-sharp-tests/DummyPapiClient.cs index d651b4de4c..dc8ac6cab2 100644 --- a/c-sharp-tests/DummyPapiClient.cs +++ b/c-sharp-tests/DummyPapiClient.cs @@ -31,6 +31,8 @@ public override Task RegisterRequestHandlerAsync( return Task.FromResult(_localMethods.TryAdd(requestType, requestHandler)); } + public IReadOnlyCollection RegisteredRequestTypes => _localMethods.Keys.ToArray(); + public override Task SendEventAsync(string eventType, object? eventParameters) { _sentEvents.Enqueue((eventType, eventParameters)); diff --git a/c-sharp-tests/Projects/LocalParatextProjectsTests.cs b/c-sharp-tests/Projects/LocalParatextProjectsTests.cs index f87e953594..f4e8f57fa2 100644 --- a/c-sharp-tests/Projects/LocalParatextProjectsTests.cs +++ b/c-sharp-tests/Projects/LocalParatextProjectsTests.cs @@ -56,4 +56,88 @@ string id { return new ProjectDetails(name, new ProjectMetadata(id, ["paratext"]), folder); } + + [Test] + public void GetParatextProjectInterfaces_Unpublished_IncludesLegacyComment() + { + var interfaces = LocalParatextProjects.GetParatextProjectInterfaces(isPublished: false); + + Assert.That(interfaces, Does.Contain(ProjectInterfaces.LEGACY_COMMENT)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.BASE)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.USFM_BOOK)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.SCRIPTURE_EDIT_PERMISSIONS)); + } + + [Test] + public void GetParatextProjectInterfaces_Published_ExcludesLegacyCommentButKeepsScripture() + { + var interfaces = LocalParatextProjects.GetParatextProjectInterfaces(isPublished: true); + + Assert.That(interfaces, Does.Not.Contain(ProjectInterfaces.LEGACY_COMMENT)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.BASE)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.USFM_BOOK)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.USFM_CHAPTER)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.USFM_VERSE)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.USX_BOOK)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.USX_CHAPTER)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.USX_VERSE)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.PLAIN_TEXT_VERSE)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.MARKER_NAMES)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.TEXT_CONNECTION_SETTINGS)); + Assert.That(interfaces, Does.Contain(ProjectInterfaces.SCRIPTURE_EDIT_PERMISSIONS)); + } + + [TestCase("ABC_4488", "PRJX", "abc7")] + public void Initialize_SingleUnpublishedProject_AdvertisesUnpublishedInterfaces( + string folder, + string name, + string id + ) + { + CreateTempProject(folder, CreateParatextProjectDetails(folder, name, id)); + _localProjects.Initialize(); + + var details = _localProjects.GetProjectDetails(id); + + // Unpublished projects must advertise the full interface list including legacyCommentManager.comments + Assert.That( + details.Metadata.ProjectInterfaces, + Does.Contain(ProjectInterfaces.LEGACY_COMMENT), + "Unpublished Paratext project must advertise legacyCommentManager.comments" + ); + Assert.That( + details.Metadata.ProjectInterfaces, + Does.Contain(ProjectInterfaces.USFM_CHAPTER) + ); + } + + [Test] + public void GetAvailableUnpublishedProjectDetails_UnpublishedOnly_ReturnsAll() + { + CreateTempProject("NR1", CreateParatextProjectDetails("NR1", "First", "aaaa01")); + CreateTempProject("NR2", CreateParatextProjectDetails("NR2", "Second", "aaaa02")); + _localProjects.Initialize(); + + var unpublished = _localProjects.GetAvailableUnpublishedProjectDetails().ToList(); + + Assert.That(unpublished, Has.Count.EqualTo(2)); + Assert.That( + unpublished.Select(d => d.Metadata.Id), + Is.EquivalentTo(new[] { "AAAA01", "AAAA02" }) + ); + } + + [Test] + public void GetAvailablePublishedProjectDetails_NoPublished_ReturnsEmpty() + { + CreateTempProject("NR1", CreateParatextProjectDetails("NR1", "First", "aaaa01")); + _localProjects.Initialize(); + + var published = _localProjects.GetAvailablePublishedProjectDetails().ToList(); + + // DummyScrText / TestLocalParatextProjectsInTempDir produces unpublished ScrTexts + // (IsResourceProject defaults to false on the base ScrText class), so the published + // list is empty in this fixture. This documents the partition between the two helpers. + Assert.That(published, Is.Empty); + } } diff --git a/c-sharp-tests/Projects/ParatextProjectDataProviderWireSurfaceTests.cs b/c-sharp-tests/Projects/ParatextProjectDataProviderWireSurfaceTests.cs new file mode 100644 index 0000000000..1debcae8db --- /dev/null +++ b/c-sharp-tests/Projects/ParatextProjectDataProviderWireSurfaceTests.cs @@ -0,0 +1,88 @@ +using System.Diagnostics.CodeAnalysis; +using Paranext.DataProvider.Projects; + +namespace TestParanextDataProvider.Projects +{ + [ExcludeFromCodeCoverage] + internal class ParatextProjectDataProviderWireSurfaceTests : PapiTestBase + { + private static readonly string[] AllCommentWireMethodSuffixes = + [ + ".getCommentThreads", + ".createComment", + ".addCommentToThread", + ".deleteComment", + ".updateComment", + ".setIsCommentThreadRead", + ".findAssignableUsers", + ".canUserCreateComments", + ".canUserAddCommentToThread", + ".canUserAssignThread", + ".canUserResolveThread", + ".canUserEditOrDeleteComment", + ]; + + [Test] + public async Task UnpublishedPdp_RegistersAllCommentMethodsAsync() + { + // Arrange: unpublished project advertises the full interface list including + // legacyCommentManager.comments + const string projId = "117777"; + var details = CreateProjectDetails( + projId, + "RegProj", + LocalParatextProjects.GetParatextProjectInterfaces(isPublished: false) + ); + ParatextProjects.FakeAddProject(details); + + // Act: construct PPDP and register it on the wire + var pdp = new ParatextProjectDataProvider("PdpA", Client, details, ParatextProjects); + await pdp.RegisterDataProviderAsync(); + + // Assert: every comment method is on the wire + foreach (var suffix in AllCommentWireMethodSuffixes) + { + Assert.That( + Client.RegisteredRequestTypes.Any(k => k.EndsWith(suffix)), + Is.True, + $"Expected unpublished PDP to register a wire method ending in '{suffix}'" + ); + } + } + + [Test] + public async Task PublishedPdp_DoesNotRegisterAnyCommentMethodAsync() + { + // Arrange: published project advertises only the published interface list (no + // legacyCommentManager.comments) + const string projId = "227777"; + var details = CreateProjectDetails( + projId, + "PublishedProj", + LocalParatextProjects.GetParatextProjectInterfaces(isPublished: true) + ); + ParatextProjects.FakeAddProject(details); + + // Act: construct PPDP and register it on the wire + var pdp = new ParatextProjectDataProvider("PdpB", Client, details, ParatextProjects); + await pdp.RegisterDataProviderAsync(); + + // Assert: no comment method appears on the wire + foreach (var suffix in AllCommentWireMethodSuffixes) + { + Assert.That( + Client.RegisteredRequestTypes.Any(k => k.EndsWith(suffix)), + Is.False, + $"Published PDP must NOT register a wire method ending in '{suffix}'" + ); + } + + // Sanity: scripture methods ARE registered (the published project is still readable) + Assert.That( + Client.RegisteredRequestTypes.Any(k => k.EndsWith(".getChapterUSFM")), + Is.True, + "Published PDP must still register scripture read methods" + ); + } + } +} diff --git a/c-sharp-tests/Projects/ParatextPublishedProjectDataProviderFactoryTests.cs b/c-sharp-tests/Projects/ParatextPublishedProjectDataProviderFactoryTests.cs new file mode 100644 index 0000000000..f2352a7d96 --- /dev/null +++ b/c-sharp-tests/Projects/ParatextPublishedProjectDataProviderFactoryTests.cs @@ -0,0 +1,45 @@ +using System.Diagnostics.CodeAnalysis; +using Paranext.DataProvider.Projects; + +namespace TestParanextDataProvider.Projects +{ + [ExcludeFromCodeCoverage] + internal class ParatextPublishedProjectDataProviderFactoryTests : PapiTestBase + { + [SetUp] + public override async Task TestSetupAsync() + { + await base.TestSetupAsync(); + } + + [Test] + public async Task PublishedFactory_AdvertisesInterfaceListWithoutLegacyCommentAsync() + { + var factory = new ParatextPublishedProjectDataProviderFactory(Client, ParatextProjects); + await factory.InitializeAsync(); + + // Truth check on the source of advertised interfaces. + var advertised = LocalParatextProjects.GetParatextProjectInterfaces(isPublished: true); + + Assert.That(advertised, Does.Not.Contain(ProjectInterfaces.LEGACY_COMMENT)); + Assert.That(advertised, Does.Contain(ProjectInterfaces.USFM_CHAPTER)); + Assert.That(advertised, Does.Contain(ProjectInterfaces.BASE)); + } + + [Test] + public async Task PublishedFactory_GetAvailableProjects_IsEmptyForDummyUnpublishedFixturesAsync() + { + // DummyScrText / FakeAddProject produces unpublished ScrTexts (IsResourceProject == + // false on the base ScrText class), so the published factory's available list is empty + // in the unit-test fixture. This is the partition assertion: unpublished projects go + // to the other factory, not this one. + const string projId = "315555"; + ParatextProjects.FakeAddProject(CreateProjectDetails(projId, "RegProj")); + + var factory = new ParatextPublishedProjectDataProviderFactory(Client, ParatextProjects); + await factory.InitializeAsync(); + + Assert.Throws(() => factory.GetProjectDataProviderID(projId)); + } + } +} diff --git a/c-sharp/Program.cs b/c-sharp/Program.cs index 4bf8a26cb6..026862d331 100644 --- a/c-sharp/Program.cs +++ b/c-sharp/Program.cs @@ -69,6 +69,10 @@ public static async Task Main() SettingsService.Initialize(papi); var paratextFactory = new ParatextProjectDataProviderFactory(papi, paratextProjects); + var paratextPublishedFactory = new ParatextPublishedProjectDataProviderFactory( + papi, + paratextProjects + ); var paratextSendReceiveService = new ParatextProjectSendReceiveService( papi, paratextFactory, @@ -80,6 +84,7 @@ public static async Task Main() var paratextRegistrationService = new ParatextRegistrationService(papi); await Task.WhenAll( paratextFactory.InitializeAsync(), + paratextPublishedFactory.InitializeAsync(), inventoryDataProvider.RegisterDataProviderAsync(), checkRunner.RegisterDataProviderAsync(), dblResources.RegisterDataProviderAsync(), diff --git a/c-sharp/Projects/LocalParatextProjects.cs b/c-sharp/Projects/LocalParatextProjects.cs index 4efca997ca..ce940c1200 100644 --- a/c-sharp/Projects/LocalParatextProjects.cs +++ b/c-sharp/Projects/LocalParatextProjects.cs @@ -1,4 +1,3 @@ -using System.Xml; using Paranext.DataProvider.ParatextUtils; using Paranext.DataProvider.Users; using Paratext.Data; @@ -32,10 +31,15 @@ internal class LocalParatextProjects "CountryStatuses.xml", ]; - private static readonly List s_paratextProjectInterfaces = + // Published projects are read-only in PT9 — ResourceProjectFileManager.SetXml() throws + // AttemptedResourceWritingException — and cannot accept comment writes. They therefore do not + // advertise legacyCommentManager.comments; everything else still applies because published + // projects can still be read for scripture and resource-references. The unpublished list is + // defined as the published list plus the comment interface so the two stay in sync by + // construction. + private static readonly List s_paratextPublishedProjectInterfaces = [ ProjectInterfaces.BASE, - ProjectInterfaces.LEGACY_COMMENT, ProjectInterfaces.USFM_BOOK, ProjectInterfaces.USFM_CHAPTER, ProjectInterfaces.USFM_VERSE, @@ -48,6 +52,12 @@ internal class LocalParatextProjects ProjectInterfaces.SCRIPTURE_EDIT_PERMISSIONS, ]; + private static readonly List s_paratextUnpublishedProjectInterfaces = + [ + .. s_paratextPublishedProjectInterfaces, + ProjectInterfaces.LEGACY_COMMENT, + ]; + public LocalParatextProjects() { ProjectRootFolder = Path.Join( @@ -95,12 +105,54 @@ public virtual void Initialize() } } + /// + /// All available Paratext projects (the union of + /// and ). Defined in terms of the partition + /// helpers so future filtering added to either partition automatically applies here too. + /// public IEnumerable GetAllProjectDetails() + { + return GetAvailableUnpublishedProjectDetails() + .Concat(GetAvailablePublishedProjectDetails()); + } + + /// + /// Available unpublished Paratext projects (regular, editable scripture projects). + /// Used by to populate its project list. + /// + public IEnumerable GetAvailableUnpublishedProjectDetails() + { + // IsResourceProject is true for ResourceScrText and JoinedScrText (PT9's read-only + // resource-backed project shapes); everything else is unpublished. + return GetVisibleScrTexts() + .Where(scrText => !scrText.IsResourceProject) + .Select(scrText => scrText.GetProjectDetails()); + } + + /// + /// Available published Paratext projects (read-only DBL / biblical resources). + /// Used by to populate its project list. + /// + public IEnumerable GetAvailablePublishedProjectDetails() + { + // IsResourceProject is true for ResourceScrText and JoinedScrText (PT9's read-only + // resource-backed project shapes). + return GetVisibleScrTexts() + .Where(scrText => scrText.IsResourceProject) + .Select(scrText => scrText.GetProjectDetails()); + } + + /// + /// Returns the set of ScrTexts that should be visible to the user given current registration + /// state. When the user has no valid Paratext registration, published projects are filtered out + /// entirely (matching PT9 behavior). + /// + private static IEnumerable GetVisibleScrTexts() { var allScrTexts = GetScrTexts(); if (!RegistrationInfo.DefaultUser.IsValid) allScrTexts = allScrTexts.Where((scrText) => !scrText.IsResourceProject); - return allScrTexts.Select(scrText => scrText.GetProjectDetails()); + return allScrTexts; } public ProjectDetails GetProjectDetails(string projectId) @@ -116,9 +168,12 @@ public static ScrText GetParatextProject(string projectId) return retVal; } - public static List GetParatextProjectInterfaces() + public static List GetParatextProjectInterfaces(bool isPublished) { - return [.. s_paratextProjectInterfaces]; + var source = isPublished + ? s_paratextPublishedProjectInterfaces + : s_paratextUnpublishedProjectInterfaces; + return [.. source]; } #endregion @@ -161,47 +216,6 @@ private static void AddProjectToScrTextCollection(ProjectDetails projectDetails) ScrTextCollection.Add(new ScrText(projectName, RegistrationInfo.DefaultUser)); } - private static ProjectDetails? LoadProjectDetails( - string projectHomeDir, - out string errorMessage - ) - { - string settingsFilePath = Path.Combine(projectHomeDir, PROJECT_SETTINGS_FILE); - if (!File.Exists(settingsFilePath)) - { - errorMessage = $"Ignoring project without Settings.xml file: {projectHomeDir}"; - return null; - } - - var settings = new XmlDocument(); - settings.Load(settingsFilePath); - - // ScrText always prioritizes the folder name over the Name setting as the "name" even when - // accessing scrText.Settings.Name. So we're copying Paratext's functionality here and using - // the folder name instead of Settings.Name. - // https://github.com/ubsicap/Paratext/blob/aaadecd828a9b02e6f55d18e4c5dda8703ce2429/ParatextData/ScrText.cs#L258 - // Removing extension twice because file may be in form name.id.ext to match Paratext - // https://github.com/ubsicap/Paratext/blob/aaadecd828a9b02e6f55d18e4c5dda8703ce2429/ParatextData/ScrTextCollection.cs#L1661 - var shortName = Path.GetFileNameWithoutExtension( - Path.GetFileNameWithoutExtension(projectHomeDir) - ); - - var idNode = settings.SelectSingleNode("/ScriptureText/Guid"); - if (idNode == null) - { - errorMessage = $"Could not find Guid in Settings.xml of {projectHomeDir}"; - return null; - } - var id = idNode.InnerText; - - var metadata = new ProjectMetadata(id, GetParatextProjectInterfaces()); - - var details = new ProjectDetails(shortName, metadata, projectHomeDir); - - errorMessage = ""; - return details; - } - private void SetUpProjectRootFolder() { CreateDirectory(ProjectRootFolder); diff --git a/c-sharp/Projects/ParatextProjectDataProvider.cs b/c-sharp/Projects/ParatextProjectDataProvider.cs index d8c3dce787..e84f20e1b3 100644 --- a/c-sharp/Projects/ParatextProjectDataProvider.cs +++ b/c-sharp/Projects/ParatextProjectDataProvider.cs @@ -47,7 +47,11 @@ internal class ParatextProjectDataProvider : ProjectDataProvider private readonly LocalParatextProjects _paratextProjects; - private readonly CommentManager _commentManager; + // Lazy because published PDPs do not register the comment wire methods (see GetFunctions), + // so for those PDPs the comment manager is never accessed - and CommentManager.Get loads + // comment XML on first access for unpublished projects, work we don't want to pay for on + // every published PDP creation. + private readonly Lazy _commentManager; private UserProjectSettings? _userProjectSettings; private string? _cachedUserId; @@ -65,8 +69,11 @@ LocalParatextProjects paratextProjects : base(name, papiClient, projectDetails) { _paratextProjects = paratextProjects; - _commentManager = CommentManager.Get( - LocalParatextProjects.GetParatextProject(projectDetails.Metadata.Id) + _commentManager = new Lazy( + () => + CommentManager.Get( + LocalParatextProjects.GetParatextProject(projectDetails.Metadata.Id) + ) ); RegisterSettingsValidators(); } @@ -93,18 +100,25 @@ LocalParatextProjects paratextProjects retVal.Add(("getVersePlainText", GetVersePlainText)); - retVal.Add(("getCommentThreads", GetCommentThreads)); - retVal.Add(("createComment", CreateComment)); - retVal.Add(("addCommentToThread", AddCommentToThread)); - retVal.Add(("deleteComment", DeleteComment)); - retVal.Add(("updateComment", UpdateComment)); - retVal.Add(("setIsCommentThreadRead", SetIsCommentThreadRead)); - retVal.Add(("findAssignableUsers", FindAssignableUsers)); - retVal.Add(("canUserCreateComments", CanUserCreateComments)); - retVal.Add(("canUserAddCommentToThread", CanUserAddCommentToThread)); - retVal.Add(("canUserAssignThread", CanUserAssignThread)); - retVal.Add(("canUserResolveThread", CanUserResolveThread)); - retVal.Add(("canUserEditOrDeleteComment", CanUserEditOrDeleteComment)); + // Comment methods are only registered when this PDP advertises legacyCommentManager.comments. + // Published PDPs do not advertise that interface (published projects are read-only and PT9 + // throws AttemptedResourceWritingException on any write to a published project), so they + // skip registration entirely instead of relying solely on per-method runtime guards. + if (ProjectDetails.Metadata.ProjectInterfaces.Contains(ProjectInterfaces.LEGACY_COMMENT)) + { + retVal.Add(("getCommentThreads", GetCommentThreads)); + retVal.Add(("createComment", CreateComment)); + retVal.Add(("addCommentToThread", AddCommentToThread)); + retVal.Add(("deleteComment", DeleteComment)); + retVal.Add(("updateComment", UpdateComment)); + retVal.Add(("setIsCommentThreadRead", SetIsCommentThreadRead)); + retVal.Add(("findAssignableUsers", FindAssignableUsers)); + retVal.Add(("canUserCreateComments", CanUserCreateComments)); + retVal.Add(("canUserAddCommentToThread", CanUserAddCommentToThread)); + retVal.Add(("canUserAssignThread", CanUserAssignThread)); + retVal.Add(("canUserResolveThread", CanUserResolveThread)); + retVal.Add(("canUserEditOrDeleteComment", CanUserEditOrDeleteComment)); + } retVal.Add(("getSetting", GetProjectSetting)); retVal.Add(("setSetting", SetProjectSetting)); @@ -213,7 +227,7 @@ protected virtual IProjectStreamManager CreateStreamManager(ProjectDetails proje public List GetCommentThreads(CommentThreadSelector selector) { // Get all threads (activeOnly=false to include threads with deleted comments) - List allThreads = _commentManager.FindThreads(activeOnly: false); + List allThreads = _commentManager.Value.FindThreads(activeOnly: false); // If no selector provided, apply defaults (exclude BT/spelling, deduplicate) selector ??= new CommentThreadSelector(); @@ -294,9 +308,9 @@ public bool DeleteComment(string commentId) VerifyUserCanEditOrDeleteComment(commentId); // Remove the comment using CommentManager - _commentManager.RemoveComment(commentToDelete); + _commentManager.Value.RemoveComment(commentToDelete); - _commentManager.SaveUser(commentToDelete.User, false); + _commentManager.Value.SaveUser(commentToDelete.User, false); SendDataUpdateEvent(AllCommentDataTypes, "comment deleted event"); return true; @@ -423,9 +437,9 @@ public string CreateComment(PlatformCommentWrapper comment) if (string.IsNullOrEmpty(newComment.Language)) newComment.Language = scrText.Language.Id; - _commentManager.AddComment(newComment); - _commentManager.SaveUser(newComment.User, false); - ThreadStatus.MarkThreadRead(_commentManager.FindThread(newComment.Thread)); + _commentManager.Value.AddComment(newComment); + _commentManager.Value.SaveUser(newComment.User, false); + ThreadStatus.MarkThreadRead(_commentManager.Value.FindThread(newComment.Thread)); SendDataUpdateEvent(AllCommentDataTypes, "comment created event"); @@ -455,7 +469,7 @@ public string AddCommentToThread(PlatformCommentWrapper comment) "At least one of Contents, Status, or AssignedUser must be provided for AddCommentToThread" ); - CommentThread? existingThread = _commentManager.FindThread(comment.Thread); + CommentThread? existingThread = _commentManager.Value.FindThread(comment.Thread); if (existingThread == null) throw new InvalidDataException($"Thread with id {comment.Thread} does not exist"); @@ -501,8 +515,8 @@ public string AddCommentToThread(PlatformCommentWrapper comment) newComment.Status = NoteStatus.Todo; } - _commentManager.AddComment(newComment); - _commentManager.SaveUser(newComment.User, false); + _commentManager.Value.AddComment(newComment); + _commentManager.Value.SaveUser(newComment.User, false); ThreadStatus.MarkThreadRead(existingThread); SendDataUpdateEvent(AllCommentDataTypes, "comment added to thread event"); @@ -581,7 +595,7 @@ public bool UpdateComment(string commentId, string updatedContentHtml) // Reset the status field to Unspecified when a comment is edited commentToUpdate.Status = NoteStatus.Unspecified; - _commentManager.SaveUser(commentToUpdate.User, false); + _commentManager.Value.SaveUser(commentToUpdate.User, false); SendDataUpdateEvent(AllCommentDataTypes, "comment updated"); @@ -590,7 +604,7 @@ public bool UpdateComment(string commentId, string updatedContentHtml) public void SetIsCommentThreadRead(string threadId, bool markRead) { - CommentThread? thread = _commentManager.FindThread(threadId); + CommentThread? thread = _commentManager.Value.FindThread(threadId); if (thread == null) throw new ArgumentException($"Thread with ID '{threadId}' not found", nameof(threadId)); @@ -723,7 +737,7 @@ private void VerifyUserCanAssignThread(string threadId) $"User '{scrText.User.Name}' does not have permission to assign this thread." ); - CommentThread? thread = _commentManager.FindThread(threadId); + CommentThread? thread = _commentManager.Value.FindThread(threadId); if (thread == null) throw new InvalidOperationException($"Thread with id {threadId} does not exist."); @@ -775,7 +789,7 @@ private void VerifyUserCanResolveThread(string threadId) "Resource projects with global note types are read-only." ); - CommentThread? thread = _commentManager.FindThread(threadId); + CommentThread? thread = _commentManager.Value.FindThread(threadId); if (thread == null) throw new InvalidOperationException($"Thread with id {threadId} does not exist."); @@ -883,7 +897,7 @@ public bool CanUserEditOrDeleteComment(string commentId) private (Comment?, CommentThread?) FindCommentByIdWithThread(string commentId) { // Get all threads (activeOnly=false to include deleted comments) - List allThreads = _commentManager.FindThreads(activeOnly: false); + List allThreads = _commentManager.Value.FindThreads(activeOnly: false); // Search through all threads to find the comment with matching ID foreach (var thread in allThreads) diff --git a/c-sharp/Projects/ParatextProjectDataProviderFactory.cs b/c-sharp/Projects/ParatextProjectDataProviderFactory.cs index ecb12b76f9..ee2952dd7d 100644 --- a/c-sharp/Projects/ParatextProjectDataProviderFactory.cs +++ b/c-sharp/Projects/ParatextProjectDataProviderFactory.cs @@ -1,99 +1,50 @@ -using System.Collections.Concurrent; using System.Text.Json; using Paranext.DataProvider.Services; +using Paratext.Data; namespace Paranext.DataProvider.Projects; -internal class ParatextProjectDataProviderFactory : ProjectDataProviderFactory +internal class ParatextProjectDataProviderFactory : ParatextProjectDataProviderFactoryBase { internal const string PDPF_NAME = "Paratext"; - private readonly LocalParatextProjects _paratextProjects; - private readonly ConcurrentDictionary _pdpMap = new(); - private readonly object _creationLock = new(); - private readonly Random _random = new((int)DateTime.Now.Ticks); public ParatextProjectDataProviderFactory( PapiClient papiClient, LocalParatextProjects paratextProjects ) - : base(LocalParatextProjects.GetParatextProjectInterfaces(), PDPF_NAME, papiClient) - { - _paratextProjects = paratextProjects; - } + : base( + papiClient, + paratextProjects, + LocalParatextProjects.GetParatextProjectInterfaces(isPublished: false), + PDPF_NAME + ) { } - protected override Task StartFactoryAsync() + protected override List? GetAvailableProjects(JsonElement _ignore) { - _paratextProjects.Initialize(); - return Task.CompletedTask; + return _paratextProjects + .GetAvailableUnpublishedProjectDetails() + .Select(pd => pd.Metadata) + .ToList(); } - protected override List? GetAvailableProjects(JsonElement _ignore) + // Published projects belong to ParatextPublishedProjectDataProviderFactory. Reject them here so + // callers don't end up with an unpublished PDP whose advertised interfaces lie about what the + // project can actually do. + protected override bool ShouldServeProject(ScrText scrText) { - return _paratextProjects.GetAllProjectDetails().Select(pd => pd.Metadata).ToList(); + return !scrText.IsResourceProject; } - public override string GetProjectDataProviderID(string projectID) + protected override string GetCrossFactoryRejectionMessage(string projectID) { - projectID = projectID.ToUpperInvariant(); - - // If we already have a PDP for this project, just return it - if (_pdpMap.TryGetValue(projectID, out var existingPdp)) - return existingPdp.DataProviderName; - - // Prevent multiple threads from trying to create PDPs at the same time - // This could probably be relaxed to be scoped per project ID, but this is more conservative - lock (_creationLock) - { - // If the PDP was created while we were locked, use it - if (_pdpMap.TryGetValue(projectID, out var existingPdpInLock)) - return existingPdpInLock.DataProviderName; - - ProjectDetails details; - try - { - details = _paratextProjects.GetProjectDetails(projectID); - } - catch (KeyNotFoundException) - { - throw new KeyNotFoundException("Unknown project ID: " + projectID); - } - - // Create a random 30 character string containing letters A-Z - var name = new string( - Enumerable.Range(0, 30).Select(_ => (char)_random.Next(65, 90)).ToArray() - ); - - // Create and store the PDP in the map for future lookups - var newPdp = new ParatextProjectDataProvider( - name, - PapiClient, - details, - _paratextProjects - ); - if (!_pdpMap.TryAdd(projectID, newPdp)) - throw new InvalidOperationException("Internal error adding project data provider"); - - // Once the PDP has been registered, return the name of it so callers can get it - ThreadingUtils.RunTask( - newPdp.RegisterDataProviderAsync(), - $"Register PDP {newPdp.DataProviderName} for project {details.Name}", - ThreadingUtils.DefaultTimeout - ); - return newPdp.DataProviderName; - } + return $"Project {projectID} is published and cannot be served by this factory"; } - /// - /// Get an existing PDP if it exists for a project id - /// - /// - /// - public ParatextProjectDataProvider? GetExistingProjectDataProvider(string projectID) + protected override string GetRegistrationTaskDescription( + ParatextProjectDataProvider pdp, + ProjectDetails details + ) { - projectID = projectID.ToUpperInvariant(); - - if (_pdpMap.TryGetValue(projectID, out var existingPdp)) - return existingPdp; - return null; + return $"Register PDP {pdp.DataProviderName} for project {details.Name}"; } } diff --git a/c-sharp/Projects/ParatextProjectDataProviderFactoryBase.cs b/c-sharp/Projects/ParatextProjectDataProviderFactoryBase.cs new file mode 100644 index 0000000000..1b42db7493 --- /dev/null +++ b/c-sharp/Projects/ParatextProjectDataProviderFactoryBase.cs @@ -0,0 +1,144 @@ +using System.Collections.Concurrent; +using System.Text.Json; +using Paranext.DataProvider.Services; +using Paratext.Data; + +namespace Paranext.DataProvider.Projects; + +/// +/// Shared base for Paratext-backed PDP factories. Concrete subclasses provide: +/// +/// the project interface list and PDPF name (constructor), +/// which projects this factory advertises +/// () and which projects this +/// factory will actually serve (), and +/// the per-factory rejection and registration log strings +/// (, +/// ). +/// +/// All other plumbing - the per-factory PDP cache, double-checked-locking creation, and existing-PDP +/// lookup - lives here so fixes and improvements apply to every Paratext PDP factory automatically. +/// +internal abstract class ParatextProjectDataProviderFactoryBase : ProjectDataProviderFactory +{ + protected readonly LocalParatextProjects _paratextProjects; + private readonly ConcurrentDictionary _pdpMap = new(); + private readonly object _creationLock = new(); + private readonly Random _random = new((int)DateTime.Now.Ticks); + + protected ParatextProjectDataProviderFactoryBase( + PapiClient papiClient, + LocalParatextProjects paratextProjects, + List projectInterfaces, + string pdpfName + ) + : base(projectInterfaces, pdpfName, papiClient) + { + _paratextProjects = paratextProjects; + } + + protected override Task StartFactoryAsync() + { + _paratextProjects.Initialize(); + return Task.CompletedTask; + } + + /// + /// Determines whether the project belongs to this factory. Used to reject projects that belong + /// to a sibling factory so callers don't end up with a PDP whose advertised interfaces lie about + /// what the project can actually do. + /// + /// + /// The discriminator is the underlying (not its advertised + /// list) so that the partition stays correct + /// even if the interface lists evolve. For example, a future restricted project type + /// (TransliterationWithEncoder) that drops legacyCommentManager.comments from + /// its advertised interfaces but is not would still + /// route to the correct factory because the check is against the ScrText flag directly. + /// + protected abstract bool ShouldServeProject(ScrText scrText); + + /// + /// Exception message used when a project belongs to a sibling factory rather than this one. + /// + protected abstract string GetCrossFactoryRejectionMessage(string projectID); + + /// + /// Short description of the PDP registration task, used for logging. Distinguishes between + /// factories so the log shows which factory registered which PDP. + /// + protected abstract string GetRegistrationTaskDescription( + ParatextProjectDataProvider pdp, + ProjectDetails details + ); + + public override string GetProjectDataProviderID(string projectID) + { + projectID = projectID.ToUpperInvariant(); + + // If we already have a PDP for this project, just return it + if (_pdpMap.TryGetValue(projectID, out var existingPdp)) + return existingPdp.DataProviderName; + + // Prevent multiple threads from trying to create PDPs at the same time + // This could probably be relaxed to be scoped per project ID, but this is more conservative + lock (_creationLock) + { + // If the PDP was created while we were locked, use it + if (_pdpMap.TryGetValue(projectID, out var existingPdpInLock)) + return existingPdpInLock.DataProviderName; + + ScrText scrText; + try + { + scrText = LocalParatextProjects.GetParatextProject(projectID); + } + catch (KeyNotFoundException) + { + throw new KeyNotFoundException("Unknown project ID: " + projectID); + } + + if (!ShouldServeProject(scrText)) + { + throw new KeyNotFoundException(GetCrossFactoryRejectionMessage(projectID)); + } + + var details = scrText.GetProjectDetails(); + + // Create a random 30 character string containing letters A-Z + var name = new string( + Enumerable.Range(0, 30).Select(_ => (char)_random.Next(65, 90)).ToArray() + ); + + // Create and store the PDP in the map for future lookups + var newPdp = new ParatextProjectDataProvider( + name, + PapiClient, + details, + _paratextProjects + ); + if (!_pdpMap.TryAdd(projectID, newPdp)) + throw new InvalidOperationException("Internal error adding project data provider"); + + // Once the PDP has been registered, return the name of it so callers can get it + ThreadingUtils.RunTask( + newPdp.RegisterDataProviderAsync(), + GetRegistrationTaskDescription(newPdp, details), + ThreadingUtils.DefaultTimeout + ); + return newPdp.DataProviderName; + } + } + + /// + /// Get an existing PDP if it exists for a project id + /// + public ParatextProjectDataProvider? GetExistingProjectDataProvider(string projectID) + { + projectID = projectID.ToUpperInvariant(); + + if (_pdpMap.TryGetValue(projectID, out var existingPdp)) + return existingPdp; + return null; + } +} diff --git a/c-sharp/Projects/ParatextPublishedProjectDataProviderFactory.cs b/c-sharp/Projects/ParatextPublishedProjectDataProviderFactory.cs new file mode 100644 index 0000000000..09b1b2445e --- /dev/null +++ b/c-sharp/Projects/ParatextPublishedProjectDataProviderFactory.cs @@ -0,0 +1,95 @@ +using System.Text.Json; +using Paranext.DataProvider.Services; +using Paratext.Data; + +namespace Paranext.DataProvider.Projects; + +/// +/// PDP factory dedicated to published Paratext projects (the same projects +/// platform.isPublished reports as true). The factory advertises every projectInterface the +/// regular factory does EXCEPT legacyCommentManager.comments, because: +/// +/// Published projects are read-only at the storage layer - +/// ResourceProjectFileManager.SetXml throws +/// AttemptedResourceWritingException on every write, so all 12 comment wire methods +/// would be guaranteed to either throw or return empty data. +/// Comments are technically readable on these projects (the reads aren't +/// storage-blocked), but no published project format we know of ships with embedded comment +/// XML. Choosing not to advertise the interface trades a theoretical read-only capability we +/// don't believe is exercised for a wire surface that's honest about what the project can do. +/// +/// +/// +/// This factory is specifically for published projects. Other PT9 project types +/// that also have restricted comment support are intentionally NOT served here: +/// +/// +/// Study Bible Additions (IsStudyBibleAdditions): can host real +/// comments. PT9's ParatextDataExtensions.CanAddNotes takes an allowInSba +/// escape parameter, used by WordListForm to create spelling notes that flow through +/// the normal CommentManager infrastructure. Stays on the regular factory; the +/// IsStudyBibleAdditions guard in +/// .VerifyUserCanCreateComments remains +/// load-bearing for the general-creation path. +/// ProjectType.TransliterationWithEncoder: has no escape hatch in +/// PT9's CanAddNotes, but we lack certainty about read-only scenarios (a project +/// converted to this type from another could still hold existing threads someone needs to +/// read). Stays on the regular factory; the TransliterationWithEncoder guard in +/// .VerifyUserCanCreateComments remains +/// load-bearing for creation. +/// Note-only project types (ConsultantNotes, +/// GlobalConsultantNotes, GlobalAnthropologyNotes): filtered out before any +/// factory sees them by ScrTextCollection.ScrTexts(IncludeProjects.ScriptureOnly). +/// Not served by any factory today; out of scope for this PR. +/// +/// +/// +/// Future restricted-interface project types should get their own dedicated PDPF +/// rather than being folded in here. This factory's contract is specifically "the data shape +/// published projects can support" - broadening it to cover other restriction patterns would +/// re-create the dishonest-interface problem this factory exists to fix. +/// +/// +internal class ParatextPublishedProjectDataProviderFactory : ParatextProjectDataProviderFactoryBase +{ + internal const string PDPF_NAME = "ParatextPublished"; + + public ParatextPublishedProjectDataProviderFactory( + PapiClient papiClient, + LocalParatextProjects paratextProjects + ) + : base( + papiClient, + paratextProjects, + LocalParatextProjects.GetParatextProjectInterfaces(isPublished: true), + PDPF_NAME + ) { } + + protected override List? GetAvailableProjects(JsonElement _ignore) + { + return _paratextProjects + .GetAvailablePublishedProjectDetails() + .Select(pd => pd.Metadata) + .ToList(); + } + + // Unpublished projects belong to ParatextProjectDataProviderFactory. Reject them here so they + // don't accidentally get served by the published factory. + protected override bool ShouldServeProject(ScrText scrText) + { + return scrText.IsResourceProject; + } + + protected override string GetCrossFactoryRejectionMessage(string projectID) + { + return $"Project {projectID} is not published and cannot be served by this factory"; + } + + protected override string GetRegistrationTaskDescription( + ParatextProjectDataProvider pdp, + ProjectDetails details + ) + { + return $"Register published PDP {pdp.DataProviderName} for project {details.Name}"; + } +} diff --git a/c-sharp/Projects/ScrTextExtensions.cs b/c-sharp/Projects/ScrTextExtensions.cs index c41a20b35f..696565bd40 100644 --- a/c-sharp/Projects/ScrTextExtensions.cs +++ b/c-sharp/Projects/ScrTextExtensions.cs @@ -10,7 +10,9 @@ internal static ProjectDetails GetProjectDetails(this ScrText scrText) scrText.Name, new( scrText.Guid.ToString().ToUpperInvariant(), - LocalParatextProjects.GetParatextProjectInterfaces() + LocalParatextProjects.GetParatextProjectInterfaces( + isPublished: scrText.IsResourceProject + ) ), scrText.Directory );