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.

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

Merge profiles on retrieve - POC and to discuss - NOT READY TO MERGE #1145

Closed
wants to merge 1 commit into from

Conversation

gdman
Copy link

@gdman gdman commented Oct 17, 2023

Profiles suck - this PR attempts to make them suck a little bit less. It's a POC and my attempt to prompt a discussion - it's in no way ready to merge.

Background

I fully expect that anyone reading a PR against this repo will know this already but lets set the scene...

When profiles are retrieved from Salesforce they only contain basic profile information i.e. description, user permissions, etc. If the same retrieve contains metadata types that have profile permissions (e.g. objects, fields, apps, classes, etc), the profile metadata will also contain permissions for that metadata. However, it'll only contain the permissions for metadata contained in that retrieve and any existing profile metadata file will be completely overwritten.

This effectively means you can't use source commands and track profiles. You kind of can, if you maintain a manifest and retrieve all metadata with permissions alongside the profiles but this isn't ideal for so many reasons.

I can understand why this has never worked - profiles are a pain in the ass and are largely superseded by Permission Sets and Permission Set Groups. In the coming years, object and field permissions will be completely removed from profiles.

So why invest time in something that doesn't have a future?

  1. It's going to be years before object permissions are completely removed from profiles (2026 I believe?)
  2. Designing new orgs with Permission Sets + Groups is very sensible but we don't always have that luxury with existing orgs and so have to work with what's there, and that's always profiles
  3. Even when object permissions are removed from profiles - there will still likely be app defaults, layout assignments, etc.

Proposal

When converting from MD API to Source format, merge retrieved profile permissions into the existing profile file. If an existing file doesn't exist, fallback to the way it works at the moment.

There would need to be configuration to determine how to merge the files, I propose the following. Two merge strategies:

  • Replace - completely replace the existing node with whatever has been retrieved. To be used for metadata that is always retrieved in profiles i.e. user permissions, description, custom, ips, etc.
  • Merge - interlace retrieved permissions with those that are already there identified by a mapping key. To be used for metadata that is selectively retrieved depending on other retrieved types i.e. field permissions, object permissions, etc.

This is an example of the configuration used in the POC:

  "profile": {
      "id": "profile",
      ...
      "strategies": {
        "adapter": "default",
        "transformer": "merged",
        "transformerConfig": {
          "rootNode": "Profile",
          "defaultHandling": {
            "strategy": "replace",
            "deleteOnEmpty": false
          },
          "nodeHandling": {
            "classAccesses": {
              "strategy": "merge",
              "mappingKey": "apexClass"
            },
            "fieldPermissions": {
              "strategy": "merge",
              "mappingKey": "field"
            },
            "loginIpRanges": {
              "strategy": "replace",
              "deleteOnEmpty": true
            },
            "objectPermissions": {
              "strategy": "merge",
              "mappingKey": "object"
            }
          }
        }
      }
    }

Note, I've also considered an attribute along the lines of 'deleteOnEmpty' (not implemented yet in the POC) to cover metadata that may genuinely have been removed from the org (opposed to just not retrieved) e.g. loginIpRanges.

This does mean there is additional configuration to be maintained. It's not ideal to maintain a list of the permission types in profiles and how to merge them. However, this is mitigated by having a default configuration of replace i.e. do what it does now. Also, the profile metadata type seldom changes.

I've tucked the functionality away behind an environment variable (SF_ENABLE_EXPERIMENTAL_PROFILE_MERGE) - having a flag to explicitly turn this feature on or off is probably a good idea. Though, since profile retrieving doesn't work at the moment, arguably a flag to turn off is probably the better way around.

Limitations / Potential Issues

  • Deletions - for example, if a field is removed from an object in the org - the retrieved profile won't contain a permission and the result of the merge will be that that field remains unchanged (not removed)
  • Sorting - we'll have to do sorting of permissions on the client side (i.e. in the library) but that may not exactly match the sorting done server side (i.e. by the MD API) so depending on method of retrieving - metadata may move around slightly

TODO

  • Discuss this change on the PR or open a discussion in forcedotcom/cli (have started here as there is code)

If the idea is a 'go'er', I'm willing to invest my time to finish off the PR.

That will involve:

  • Fixing all the TypeScript compile and es-lint errors (I've littered it with ignores to make the build compile, I rarely write TypeScript... and so far I'm not rushing to start a new TypeScript project anytime soon...)
  • Add remaining nodeHandling configurations
  • Implement deleteOnEmpty
  • Write unit tests
  • Reviews

Conclusion

Ultimately, (and I mean no offence to anyone) I don't think we can make the handling of profiles much worse than it already is - therefore, there is no downside to giving this a shot.

Before:
Before

After:
After

…nd to elicit feedback - NOT READY TO MERGE

TODO:
- Get concept reviewed
- Fix all the TypeScript compile and es-lint errors (yes, I suck at TypeScript, it's a hateful language)
- Add remaining nodeHandling configurations
- Implement deleteOnEmpty (see comment in mergedMetadataTransformer.ts)
- Write unit tests
@gdman gdman requested a review from a team as a code owner October 17, 2023 15:12
@mshanemc
Copy link
Contributor

mshanemc commented Oct 30, 2023

@gdman We've been discussing this PR some internally. We also have another proposal that kinda might be closer to what you want for Profiles, but be more broadly useful: forcedotcom/cli#2544

It would need some way (possibly a new command) to align profiles with other metadata if there's been deletes (ex: a deleted field). I had a command like that in my ill-maintained personal plugins...it would compare profile/permSets to the object/field/class etc and remove stuff that wasn't there in the local project.

That proposal does not have a "bring-your-own-transformer/adapter" so you're constrained to using the existing ones.

@mshanemc
Copy link
Contributor

mshanemc commented Apr 8, 2024

We're going to do something similar as part of forcedotcom/cli#2544

@mshanemc mshanemc closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants