Skip to content
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

Fix #652: Don't create mappings unless they're actually needed #656

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Fix #652: Don't create mappings unless they're actually needed #656

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
478042d
fix: don't add needless mappings
isc-tleavitt Dec 11, 2024
43e9957
chore (docs): tweak changelog
isc-tleavitt Dec 11, 2024
4f77156
chore (docs): update method description
isc-tleavitt Dec 11, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- #559: Allow treating the "w" in SemVer x.y.z-w as a post-release rather than pre-release.
- #607: Uninstall reports deletion of non-classes
- #606: Don't put garbage folders in tar archive
- #652: Don't create extra needless mappings (could cause deadlock with parallel installation of dependencies)

### Security
-
Expand Down
2 changes: 1 addition & 1 deletion src/cls/IPM/ResourceProcessor/Default/Class.cls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ Method OnConfigureMappings(ByRef pParams) As %Status
Set tName = $Piece(..ResourceReference.Name,".",1,*-1)
Set tGlobalScope = ..ResourceReference.Module.GlobalScope && '$Get(pParams("Reload","ForceLocalScope"),0)
If 'tGlobalScope {
Set tNeedExplicitMapping = ..FileExistsInCurrentNS(tName)
Set tNeedExplicitMapping = ..ResourceIsMappedToDefaultDB(..ResourceReference.Name)
If tNeedExplicitMapping {
Set tPackage = $p(tName,".",1,*-1)
Set tSourceDB = ##class(%IPM.Utils.Module).GetRoutineDatabase($Namespace)
Expand Down
29 changes: 13 additions & 16 deletions src/cls/IPM/ResourceProcessor/Default/Document.cls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,23 +609,20 @@ Method OnGetUniqueName(Output pUniqueName)

/// Returns a boolean of whether current file exists in the the routine database of the current namespace exists in current namespace's database
isc-tleavitt marked this conversation as resolved.
Show resolved Hide resolved
/// Helper method to be used in derived classes' OnConfigureMappings() to skip creating unnecessary mapping
ClassMethod FileExistsInCurrentNS(pFileName As %String) As %Boolean
ClassMethod ResourceIsMappedToDefaultDB(pResourceName As %String) As %Boolean
{
Set tSourceDB = ##class(%IPM.Utils.Module).GetRoutineDatabase($Namespace)
// Check if current file exists in the the routine database of the current namespace exists in current namespace('s database)
// If yes, skip creating unnecessary mappings
// (Spec,Dir=1,OrderBy=1,SystemFiles=1,Flat,NotStudio=0,ShowGenerated=0,Filter,RoundTime=0,Mapped=0)
// Set mapped=0 since we only want to check whether the file exists in the current routine DB
Set tResult = ##class(%Library.RoutineMgr).StudioOpenDialogFunc(pFileName_".*",1,1,1,,,,,,0)
Set tNeedExplicitMapping = 1
While tResult.%Next(.tSC) {
$$$ThrowOnError(tSC)
if ($ZConvert(tResult.%Get("Name"),"U") = $ZConvert(pFileName, "U")) {
Set tNeedExplicitMapping = 0
}
}
$$$ThrowOnError(tSC)
Return tNeedExplicitMapping
Set defaultDB = ##class(%IPM.Utils.Module).GetRoutineDatabaseDir($Namespace)
Set name = $Piece(pResourceName,".",1,*-1)
Set type = $ZConvert($Piece(pResourceName,".",*),"U")
If (type = "PKG") {
Set db = ##class(%SYS.Namespace).GetPackageDest(,name)
} ElseIf (type = "CLS") {
Set db = ##class(%SYS.Namespace).GetPackageDest(,$Piece(name,".",1,*-1))
} Else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a case for globals as well? Also, rather than grouping the specific use cases in the Document super class, would it make sense to have specific implementations in the respective sub classes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was interesting looking back at the history of Beiming's work here - it started out as a separate implementations, then moved to a centralized (wrong in many ways) implementation.

In this case I focused on making it work for all of the existing callers, which all now pass an InternalName. Agreed that we should branch out to globals later; filed a separate issue for this: #658

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A reason to keep this centralized based on InternalName is that a custom resource processor might care about the mapping of something other than the document name itself (e.g., an associated/generated file).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that case, a custom processor can call the method for the corresponding file type though right instead of calling the one in Document? I think specifically just the call to Get*Dest/equivalent should be in separate sub-classes of Document so the overarching logic for can remain in Document since that's the only part that differs between different document types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair - will cover refactoring in the followup task as well.

Set db = ##class(%SYS.Namespace).GetRoutineDest(,name)
}
Set db = $Piece(db,"^",2,*) // Slight difference in format of reporting here
Return (db = defaultDB)
}

}
2 changes: 1 addition & 1 deletion src/cls/IPM/ResourceProcessor/Default/LocalizedMessages.cls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ Method OnConfigureMappings(ByRef pParams) As %Status
Set tName = $Piece(..ResourceReference.Name,".",1,*-1)
Set tGlobalScope = ..ResourceReference.Module.GlobalScope && '$Get(pParams("Reload","ForceLocalScope"),0)
If 'tGlobalScope {
Set tNeedExplicitMapping = ..FileExistsInCurrentNS(tName)
Set tNeedExplicitMapping = ..ResourceIsMappedToDefaultDB(tName_".INC")
If tNeedExplicitMapping {
Set tSourceDB = ##class(%IPM.Utils.Module).GetRoutineDatabase($Namespace)
Set tSC = ##class(%IPM.Utils.Module).AddRoutineMapping($namespace,tName,,tSourceDB)
Expand Down
4 changes: 2 additions & 2 deletions src/cls/IPM/ResourceProcessor/Default/Package.cls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Property Directory As %String(MAXLEN = "") [ InitialExpression = "cls" ];
Property LoadAsDirectory As %Boolean [ InitialExpression = 1 ];

/// Extension for individual filename(s) that comprise this resource
Property FilenameExtension As %String [ InitialExpression = "" ];
Property FilenameExtension As %String;

/// Subclasses may override to customize mapping behavior at the beginning of the Reload phase.
Method OnConfigureMappings(ByRef pParams) As %Status
Expand All @@ -26,7 +26,7 @@ Method OnConfigureMappings(ByRef pParams) As %Status
Set tName = $Piece(..ResourceReference.Name,".",1,*-1)
Set tGlobalScope = ..ResourceReference.Module.GlobalScope && '$Get(pParams("Reload","ForceLocalScope"),0)
If 'tGlobalScope {
Set tNeedExplicitMapping = ..FileExistsInCurrentNS(tName)
Set tNeedExplicitMapping = ..ResourceIsMappedToDefaultDB(..ResourceReference.Name)
If tNeedExplicitMapping {
Set tSourceDB = ##class(%IPM.Utils.Module).GetRoutineDatabase($Namespace)
Set tSC = ##class(%IPM.Utils.Module).AddPackageMapping($namespace,tName,tSourceDB)
Expand Down
2 changes: 1 addition & 1 deletion src/cls/IPM/ResourceProcessor/Default/Routine.cls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Method OnConfigureMappings(ByRef pParams) As %Status
Set tName = $Piece(..ResourceReference.Name,".",1,*-1)
Set tGlobalScope = ..ResourceReference.Module.GlobalScope && '$Get(pParams("Reload","ForceLocalScope"),0)
If 'tGlobalScope && '..LoadAsDirectory {
Set tNeedExplicitMapping = ..FileExistsInCurrentNS(tName)
Set tNeedExplicitMapping = ..ResourceIsMappedToDefaultDB(..ResourceReference.Name)
If tNeedExplicitMapping {
Set tSourceDB = ##class(%IPM.Utils.Module).GetRoutineDatabase($Namespace)
Set tSC = ##class(%IPM.Utils.Module).AddRoutineMapping($namespace,tName,,tSourceDB)
Expand Down
Loading