-
Notifications
You must be signed in to change notification settings - Fork 125
fix: adds support for nested InFolder metadata types #455
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.
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
Changes from all commits
6df3548
c1e0665
dcf7d04
ff99117
02924a0
3ce4ecd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,9 @@ import { MetadataRegistry, MetadataType } from './types'; | |
export class RegistryAccess { | ||
private registry: MetadataRegistry; | ||
|
||
private strictFolderTypes: MetadataType[]; | ||
private folderContentTypes: MetadataType[]; | ||
|
||
constructor(registry: MetadataRegistry = defaultRegistry) { | ||
this.registry = registry; | ||
} | ||
|
@@ -71,9 +74,31 @@ export class RegistryAccess { | |
* @returns An array of metadata type objects that require strict parent folder names | ||
*/ | ||
public getStrictFolderTypes(): MetadataType[] { | ||
return Object.values(this.registry.strictDirectoryNames).map( | ||
(typeId) => this.registry.types[typeId] | ||
); | ||
if (!this.strictFolderTypes) { | ||
this.strictFolderTypes = Object.values(this.registry.strictDirectoryNames).map( | ||
(typeId) => this.registry.types[typeId] | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only need to do this once. The registry data can't change once RegistryAccess is created. |
||
} | ||
return this.strictFolderTypes; | ||
} | ||
|
||
/** | ||
* Query for the types that have the folderContentType property defined. | ||
* E.g., reportFolder, dashboardFolder, documentFolder, emailFolder | ||
* @see {@link MetadataType.folderContentType} | ||
* | ||
* @returns An array of metadata type objects that have folder content | ||
*/ | ||
public getFolderContentTypes(): MetadataType[] { | ||
if (!this.folderContentTypes) { | ||
this.folderContentTypes = []; | ||
for (const type of Object.values(this.registry.types)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think this is why the registry has so many other properties? To avoid the perf hit of iterating over every type just to find some that have a property defined? Would it make sense to add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By "so many other properties" are you referring to the indexes like suffixes, strictDirectoryNames, and childTypes? If so, yeah that's for better perf. There are only 4 folderContentTypes so I don't think it's worth indexing them. Building the array once makes sense to me though. I think RegistryAccess could use more memoization logic like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah exactly. I'd prefer not to iterate over the whole registry to find four entries, multiple times... but we could do this optimization later There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's only done once, not multiple times. |
||
if (type.folderContentType) { | ||
this.folderContentTypes.push(type); | ||
} | ||
} | ||
} | ||
return this.folderContentTypes; | ||
} | ||
|
||
get apiVersion(): string { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
* For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
*/ | ||
|
||
import { RegistryAccess } from '../registry'; | ||
import { MetadataType, RegistryAccess } from '../registry'; | ||
import { NodeFSTreeContainer, TreeContainer } from './treeContainers'; | ||
import { MetadataComponent } from './types'; | ||
import { parse as parseXml } from 'fast-xml-parser'; | ||
|
@@ -64,16 +64,46 @@ export class ManifestResolver { | |
const typeName = typeMembers.name; | ||
const type = this.registry.getTypeByName(typeName); | ||
const parentType = type.folderType ? this.registry.getTypeByName(type.folderType) : undefined; | ||
for (const fullName of normalizeToArray(typeMembers.members)) { | ||
const members = normalizeToArray(typeMembers.members); | ||
|
||
for (const fullName of members) { | ||
let mdType = type; | ||
// if there is no / delimiter and it's a type in folders that aren't nestedType, infer folder component | ||
if (type.folderType && !fullName.includes('/') && parentType.folderType !== parentType.id) { | ||
mdType = this.registry.getTypeByName(type.folderType); | ||
if (this.isNestedInFolder(fullName, type, parentType, members)) { | ||
mdType = parentType; | ||
} | ||
components.push({ fullName, type: mdType }); | ||
} | ||
} | ||
|
||
return { components, apiVersion }; | ||
} | ||
|
||
// Use the folderType instead of the type from the manifest when: | ||
// 1. InFolder types: (report, dashboard, emailTemplate, document) | ||
// 1a. type.inFolder === true (from registry.json) AND | ||
// 1b. The fullName doesn't contain a forward slash character AND | ||
// 1c. The fullName with a slash appended is contained in another member entry | ||
// OR | ||
// 2. Non-InFolder, folder types: (territory2, territory2Model, territory2Type, territory2Rule) | ||
// 2a. type.inFolder !== true (from registry.json) AND | ||
// 2b. type.folderType has a value (from registry.json) AND | ||
// 2c. This type's parent type has a folderType that doesn't match its ID. | ||
private isNestedInFolder( | ||
fullName: string, | ||
type: MetadataType, | ||
parentType: MetadataType, | ||
members: string[] | ||
): boolean { | ||
// Quick short-circuit for non-folderTypes | ||
if (!type.folderType) { | ||
return false; | ||
} | ||
|
||
const isInFolderType = type.inFolder; | ||
const isNestedInFolder = | ||
!fullName.includes('/') || members.some((m) => m.includes(`${fullName}/`)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can windows users put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that's valid. It's not a file path, which would be OS specific. It's the server-assigned API name. |
||
const isNonMatchingFolder = parentType && parentType.folderType !== parentType.id; | ||
|
||
return (isInFolderType && isNestedInFolder) || (!isInFolderType && isNonMatchingFolder); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding these back since they are returned by a describemetadata call, and that's what we should be using as the source of truth for type definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source of untruth