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

Breaking changes wishlist #328

Closed
ascott18 opened this issue Oct 17, 2023 · 2 comments
Closed

Breaking changes wishlist #328

ascott18 opened this issue Oct 17, 2023 · 2 comments
Labels

Comments

@ascott18
Copy link
Collaborator

ascott18 commented Oct 17, 2023

Breaking (major)

  • IN: Convert methods that return a tuple of (ItemResult, IncludeTree) to just use the include tree prop on ItemResult<>.
  • IN: Change AfterSave to return an ItemResult, remove ref includetree, make item not a ref. Add an async variant.
  • IN: Change Vue API callers to return the data inside the ItemResult/ListResult, instead of returning the raw axios response. Move the raw axios response to a property on the API caller.

Breaking (minor)

  • OUT: Change c-input default for booleans to checkbox instead of switch
  • IN(INVESTIGATE): Remove ToLower operation in string search operations (if this is harming DB performance. TODO: compare query plans between sql with the lowering operation and SQL without it to see if there's a plan difference when the searched column is indexed and the search operation can use the index (StartsWith and Equals).
  • WAIT FOR NEED: Hooks in Behaviors for bulk saves (these don't have to be breaking changes, could just be empty default implementations on IBehaviors)
    • Hooks for after full operation is done
    • Hooks for before full operation begins ("pre-validation"?)
  • IN: Rename $withSimultaneousRequestCaching to $useSimultaneousRequestCaching. Also consider adding a version that is specific to an ApiCaller instance, for parity with features like $useResponseCaching.
  • MOVE TO TEMPLATE: IntelliTect.Coalesce.Utilities.ClaimsPrincipalExtensions. These methods make great assumptions about the authentication mechanisms used by an application, which is something that Coalesce is generally agnostic to. Just had a bug in ICMS because GetRoles() assumes the claim type used for roles.

Deprecations

  • IN: Deprecate DtoIncludesAttribute and DtoExcludesAttribute
    • 99% of the time, it is much better to use an appropriately customized data source. The includes/exclude attributes encourage loading of data from the database that will just be thrown away during mapping.
    • In the worst case, RestrictAttribute can do anything these attributes can do since restrictions have access to the includes string through the mapping context.
  • IN: Deprecate SelectFilter
    • The Vue stack does not support this attribute and never has, this attribute does not enforce any kind of backend validation (but may give the illusion that it does). If your dropdowns have complex logic for what values are allowed in them, you probably should be writing custom pages (this attribute only affects the generated knockout admin pages).
    • Alternatively, we could add support to the vue stack, but I haven't heard any demands for it.
  • IN: TypeScriptPartial
    • This is distinctly knockout only. At the very least, we should move it elsewhere in the documentation so it isn't cluttering up the main attribute documentation.
  • IN: [ControllerAttribute]. Added for IPC. Unused by every other project.
  • IN: [CreateControllerAttribute]. WillCreateView=false is knockout only. WillCreateApi=false is redundant with adding Deny permission to Read, Create, Edit, Delete. Globally disabling kinds of generated code for all types (e.g. disable all views or all API controllers) can be done via code gen config (coalesce.json).
  • IN: [LoadFromDataSourceAttribute]. Niche attribute, very rarely used. Merge into ExecuteAttribute?
@ascott18
Copy link
Collaborator Author

ToLower operation in string search operations:

  • On non-indexed columns, removing this drops a ComputeScalar step in the query plan when searching on multiple columns
  • On a search of both one and multiple indexed columns, query plans are identical

Doesn't seem like a big enough impact to be worth changing this now.

@ascott18
Copy link
Collaborator Author

Of all the attributes we're considering deprecating, DtoIncludes and DtoExcludes are used far and away the most in existing projects. I've changed my mind on these and will not be deprecating them at this time - their alternative of creating a whole custom Restriction feels a bit too heavy of a migration path.

ascott18 added a commit that referenced this issue Jul 15, 2024
ascott18 added a commit that referenced this issue Jul 16, 2024
… ListResult/ItemResult instead of returning the outer axios response. The raw response of the previous request is now available on `.rawResponse`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant