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

feat/refactor: dedicated classes for texts and inline comps #356

Merged
merged 33 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
07dbaf8
refactor: overhaul/fix creation of `drafts` properties in entities
stockbal Oct 5, 2024
8260c32
refactor: rename cds file that causes transpilation errors
stockbal Oct 5, 2024
8d2b697
fix: fixes lint issues
stockbal Oct 5, 2024
6050b06
refactor: remove xtended csn
stockbal Oct 10, 2024
ac1552f
fix: transfer `key` value of associations to foreign key element
stockbal Oct 10, 2024
1c5844c
fix: do not skip inherited elements for views/projections
stockbal Oct 10, 2024
383b541
fix: fixes inline composition resolution
stockbal Oct 10, 2024
cf955ff
fix: fixes scoped entities
stockbal Oct 11, 2024
9e48f81
feat: include texts entities
stockbal Oct 11, 2024
61a429f
test: adjust tests to handle entities in subnamespacees
stockbal Oct 11, 2024
935995e
refactor: remove unused imports
stockbal Oct 12, 2024
4b8f1ec
Merge branch 'main' into refactor/one-csn
stockbal Oct 23, 2024
a2034a2
Merge branch 'main' into refactor/one-csn
stockbal Nov 23, 2024
3c8c5b4
fix: imports/exports for esm mode
stockbal Nov 23, 2024
f55e26f
Merge branch 'main' into refactor/one-csn
stockbal Dec 1, 2024
dc56572
feat: propagate inflection annotations from xtended
stockbal Dec 1, 2024
2d00a01
fix: fix output test
stockbal Dec 1, 2024
f2d462a
docs: add preliminary changelog
stockbal Dec 1, 2024
6d17ed5
feat: print inline enum to subnamespace buffer if necessary
stockbal Dec 3, 2024
e2d7756
Merge branch 'main' into refactor/one-csn
stockbal Dec 4, 2024
9524c6a
fix: use index access for inline entities in js exports
stockbal Dec 4, 2024
b274d05
fix: do not discard of inflection annotations exclusive to inferred csn
stockbal Dec 4, 2024
05b26c1
test: add test cases
stockbal Dec 4, 2024
f4080c3
refactor: cache inherited elements of entity inside repository
stockbal Dec 6, 2024
e1957ed
test: add/enhance test for inline compositions
stockbal Dec 6, 2024
a71c475
Merge branch 'main' into refactor/one-csn
stockbal Dec 6, 2024
d17f8ba
Merge branch 'main' into refactor/one-csn
stockbal Dec 12, 2024
4b0f3d3
Merge branch 'main' into refactor/one-csn
stockbal Dec 17, 2024
7e8d370
chore: update changelog
stockbal Dec 17, 2024
9b47593
Merge branch 'main' into refactor/one-csn
daogrady Jan 8, 2025
8ac088c
refactor: resolve review remarks
stockbal Jan 8, 2025
c6ea598
refactor: resolve review remarks
stockbal Jan 8, 2025
d1aa000
Merge branch 'main' into refactor/one-csn
daogrady Jan 9, 2025
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
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ All notable changes to this project will be documented in this file.

## [Unreleased]

### Added
### Added
- dedicated classes for inline compositions
- dedicated text-classes for entities with `localized` elements

### Changed
- prefixed builtin types like `Promise` and `Record` with `globalThis.`, to allow using names of builtin types for entities without collisions
### Deprecated
Expand Down
26 changes: 26 additions & 0 deletions lib/csn.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const { LOG } = require('./logging')
const { annotations } = require('./util')

const DRAFT_ENABLED_ANNO = '@odata.draft.enabled'
/** @type {string[]} */
Expand Down Expand Up @@ -292,6 +293,30 @@ function propagateForeignKeys(csn) {
}
}

/**
* Clears "correct" singular/plural annotations from inferred model
* copies the ones from the xtended model.
*
* This is done to prevent potential duplicate class names because of annotation propagation.
* @param {{inferred: CSN, xtended: CSN}} csn - CSN models
*/
function propagateInflectionAnnotations(csn) {
const singularAnno = annotations.singular[0]
const pluralAnno = annotations.plural[0]
for (const [name, def] of Object.entries(csn.inferred.definitions)) {
const xtendedDef = csn.xtended.definitions[name]
// we keep the annotations from definition specific to the inferred model (e.g. inline compositions)
if (!xtendedDef) continue

// clear annotations from inferred definition
if (Object.hasOwn(def, singularAnno)) delete def[singularAnno]
if (Object.hasOwn(def, pluralAnno)) delete def[pluralAnno]
// transfer annotation from xtended if existing
if (Object.hasOwn(xtendedDef, singularAnno)) def[singularAnno] = xtendedDef[singularAnno]
if (Object.hasOwn(xtendedDef, pluralAnno)) def[pluralAnno] = xtendedDef[pluralAnno]
}
}

/**
* @param {EntityCSN} entity - the entity
*/
Expand Down Expand Up @@ -326,5 +351,6 @@ module.exports = {
getProjectionAliases,
getViewTarget,
propagateForeignKeys,
propagateInflectionAnnotations,
isCsnAny
}
43 changes: 35 additions & 8 deletions lib/file.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ class SourceFile extends File {
if (!(name in this.namespaces)) {
const buffer = new Buffer()
buffer.closed = false
buffer.namespace = name
buffer.add(`export namespace ${name} {`)
buffer.indent()
this.namespaces[name] = buffer
Expand Down Expand Up @@ -286,6 +287,8 @@ class SourceFile extends File {
* @param {string} entityFqName - name of the entity the enum is attached to with namespace
* @param {string} propertyName - property to which the enum is attached.
* @param {[string, string][]} kvs - list of key-value pairs
* @param {Buffer} [buffer] - if buffer is of subnamespace the enum will be added there,
* otherwise to the inline enums of the file
* @param {string[]} doc - the enum docs
* If given, the enum is considered to be an inline definition of an enum.
* If not, it is considered to be regular, named enum.
Expand All @@ -310,16 +313,32 @@ class SourceFile extends File {
* }
* ```
*/
addInlineEnum(entityCleanName, entityFqName, propertyName, kvs, doc=[]) {
addInlineEnum(entityCleanName, entityFqName, propertyName, kvs, buffer, doc=[]) {
const namespacedEntity = [buffer?.namespace, entityCleanName].filter(Boolean).join('.')
this.enums.data.push({
name: `${entityCleanName}.${propertyName}`,
name: `${namespacedEntity}.${propertyName}`,
property: propertyName,
kvs,
fq: `${entityCleanName}.${propertyName}`
fq: `${namespacedEntity}.${propertyName}`
})
const entityProxy = this.entityProxies[entityCleanName] ?? (this.entityProxies[entityCleanName] = [])
const entityProxy = this.entityProxies[namespacedEntity] ?? (this.entityProxies[namespacedEntity] = [])
entityProxy.push(propertyName)
printEnum(this.inlineEnums.buffer, propertyToInlineEnumName(entityCleanName, propertyName), kvs, {export: false}, doc)

// REVISIT: find a better way to do this???
const printEnumToBuffer = (/** @type {Buffer} */buffer) => printEnum(buffer, propertyToInlineEnumName(entityCleanName, propertyName), kvs, {export: false}, doc)

if (buffer?.namespace) {
const tempBuffer = new Buffer()
// we want to put the enums on class level
tempBuffer.indent()
printEnumToBuffer(tempBuffer)

// we want to write the enums at the beginning of the namespace
daogrady marked this conversation as resolved.
Show resolved Hide resolved
const [first,...rest] = buffer.parts
buffer.parts = [first, ...tempBuffer.parts, ...rest]
} else {
printEnumToBuffer(this.inlineEnums.buffer)
}
}

/**
Expand Down Expand Up @@ -490,12 +509,15 @@ class SourceFile extends File {

return {
singularRhs: `createEntityProxy(['${namespace}', '${original}'], { target: { is_singular: true }${customPropsStr} })`,
pluralRhs: `createEntityProxy(['${namespace}', '${original}'])`,
pluralRhs: `createEntityProxy(['${namespace}', '${original}'], { target: { is_singular: false }})`,
}
} else {
// standard entity: csn.Books
// inline entity: csn['Books.texts']
const csnAccess = original.includes('.') ? `csn['${original}']` : `csn.${original}`
return {
singularRhs: `{ is_singular: true, __proto__: csn.${original} }`,
pluralRhs: `csn.${original}`
singularRhs: `{ is_singular: true, __proto__: ${csnAccess} }`,
pluralRhs: csnAccess
}
}
}
Expand Down Expand Up @@ -589,6 +611,11 @@ class Buffer {
* @type {boolean}
*/
this.closed = false
/**
* Required for inline enums of inline compositions or text entities
* @type {string | undefined}
*/
this.namespace = undefined
}

/**
Expand Down
6 changes: 4 additions & 2 deletions lib/printers/javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,14 @@ class ESMPrinter extends JavaScriptPrinter {

/** @type {JavaScriptPrinter['printDeconstructedImport']} */
printDeconstructedImport (imports, from) {
return `import { ${imports.join(', ')} } from '${from}'`
return `import { ${imports.join(', ')} } from '${from}/index.js'`
}

/** @type {JavaScriptPrinter['printExport']} */
printExport (name, value) {
return `export const ${name} = ${value}`
return name.includes('.')
? `${name} = ${value}`
: `export const ${name} = ${value}`
}

/** @type {JavaScriptPrinter['printDefaultExport']} */
Expand Down
16 changes: 16 additions & 0 deletions lib/resolution/entity.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,22 @@ class EntityInfo {
/** @type {import('../typedefs').resolver.EntityCSN | undefined} */
#csn

/** @type {Set<string> | undefined} */
#inheritedElements

/** @returns set of inherited elements (e.g. ID of aspect cuid) */
get inheritedElements() {
Copy link
Contributor

@daogrady daogrady Jan 8, 2025

Choose a reason for hiding this comment

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

Have you considered property renaming in this context? I.e., does your code exhibit well-defined behaviour for when a property is present in a parent, but also in the entity itself (but possibly with a changed type)?

entity A {
  name:String
}

entity B: A {
  name:Integer
}

As of today, this is a known restriction. Even if you do not explicitly solve the issue, this could be an opportunity to error out if we find redefined properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I haven't considered them - why would anyone do that 😶‍🌫️. No proper solution comes to mind right away, I would have to think about it some more.
If I understand you correctly I could implement the following approach though, to error out and stop the type generation in case of a type mismatch.

// visitor.js - #aspectify
  for (let [ename, element] of Object.entries(entity.elements ?? [])) {
    const inheritedElem = inheritedElements?.get(ename)
    if (inheritedElem) {
        const type = stringifyType(element.type)
        const inheritedElemType = stringifyType(inheritedElem.type)
        if (stringifiedType === inheritedElemType) {
            continue
        } else {
            throw new Error(`Type '${type}' of element '${entity.name}.${ename}' does not match the type '${inheritedElemType}' from the parent`)
        }
    }
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what the architectural reasons would be, but it has happened in the past. 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S.: I asked this mainly out of curiosity. As it is beyond the scope of your PR, this can totally be done at a later point in time and by someone else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I would actually table this for later if this is ok for you

Copy link
Contributor

Choose a reason for hiding this comment

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

Absolutely! 🙂

if (this.#inheritedElements) return this.#inheritedElements
this.#inheritedElements = new Set()
for (const parentName of this.csn.includes ?? []) {
const parent = this.#repository.getByFq(parentName)
for (const element of Object.keys(parent?.csn?.elements ?? {})) {
this.#inheritedElements.add(element)
}
}
return this.#inheritedElements
}

/** @returns the **inferred** csn for this entity. */
get csn () {
return this.#csn ??= this.#resolver.csn.definitions[this.fullyQualifiedName]
Expand Down
39 changes: 19 additions & 20 deletions lib/resolution/resolver.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const { configuration } = require('../config')
/** @typedef {{typeName: string, typeInfo: TypeResolveInfo & { inflection: Inflection }}} ResolveAndRequireInfo */

class Resolver {
get csn() { return this.visitor.csn.inferred }
get csn() { return this.visitor.csn }

/** @param {Visitor} visitor - the visitor */
constructor(visitor) {
Expand Down Expand Up @@ -165,11 +165,13 @@ class Resolver {
*/
const isPropertyOf = (property, entity) => property && Object.hasOwn(entity?.elements ?? {}, property)

const defs = this.visitor.csn.inferred.definitions
const defs = this.visitor.csn.definitions

// check if name is already an entity, then we do not have a property access, but a nested entity
if (defs[p]?.kind === 'entity') return []

// assume parts to contain [Namespace, Service, Entity1, Entity2, Entity3, property1, property2]
/** @type {string} */
// @ts-expect-error - nope, we know there is at least one element
let qualifier = parts.shift()
let qualifier = /** @type {string} */ (parts.shift())
// find first entity from left (Entity1)
while ((!defs[qualifier] || !isEntity(defs[qualifier])) && parts.length) {
qualifier += `.${parts.shift()}`
Expand Down Expand Up @@ -240,6 +242,8 @@ class Resolver {
} else {
// TODO: make sure the resolution still works. Currently, we only cut off the namespace!
plural = util.getPluralAnnotation(typeInfo.csn) ?? typeInfo.plainName
// remove leading entity name
if (plural.includes('.')) plural = last(plural)
singular = util.getSingularAnnotation(typeInfo.csn) ?? util.singular4(typeInfo.csn, true) // util.singular4(typeInfo.csn, true) // can not use `plural` to honor possible @singular annotation

// don't slice off namespace if it isn't part of the inflected name.
Expand Down Expand Up @@ -311,18 +315,6 @@ class Resolver {
} else {
let { singular, plural } = targetTypeInfo.typeInfo.inflection

// FIXME: super hack!!
// Inflection currently does not retain the scope of the entity.
daogrady marked this conversation as resolved.
Show resolved Hide resolved
// But we can't just fix it in inflection(...), as that would break several other things
// So we bandaid-fix it back here, as it is the least intrusive place -- but this should get fixed asap!
if (target.type) {
const untangled = this.visitor.entityRepository.getByFqOrThrow(target.type)
const scope = untangled.scope.join('.')
if (scope && !singular.startsWith(scope)) {
singular = `${scope}.${singular}`
}
}

typeName = cardinality > 1
? toMany(plural)
: toOne(this.visitor.isSelfReference(target) ? 'this' : singular)
Expand Down Expand Up @@ -370,8 +362,14 @@ class Resolver {
// handle typeof (unless it has already been handled above)
const target = element.target?.name ?? element.type?.ref?.join('.') ?? element.type
if (target && !typeInfo.isDeepRequire) {
const { propertyAccess } = this.visitor.entityRepository.getByFq(target) ?? {}
if (propertyAccess?.length) {
const { propertyAccess, scope } = this.visitor.entityRepository.getByFq(target) ?? {}
if (scope?.length) {
// update inflections with proper prefix, e.g. Books.text, Books.texts
typeInfo.inflection = {
singular: [...scope, typeInfo.inflection?.singular].join('.'),
plural: [...scope, typeInfo.inflection?.plural].join('.')
}
} else if (propertyAccess?.length) {
const element = target.slice(0, -propertyAccess.join('.').length - 1)
const access = this.visitor.inlineDeclarationResolver.getTypeLookup(propertyAccess)
// singular, as we have to access the property of the entity
Expand Down Expand Up @@ -452,6 +450,7 @@ class Resolver {

const cardinality = getMaxCardinality(element)

/** @type {TypeResolveInfo} */
const result = {
isBuiltin: false, // will be rectified in the corresponding handlers, if needed
isInlineDeclaration: false,
Expand Down Expand Up @@ -569,7 +568,7 @@ class Resolver {
* @returns @see resolveType
*/
resolveTypeName(t, into) {
const result = into ?? {}
const result = into ?? /** @type {TypeResolveInfo} */({})
const path = t.split('.')
const builtin = this.builtinResolver.resolveBuiltin(path)
if (builtin === undefined) {
Expand Down
4 changes: 3 additions & 1 deletion lib/typedefs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export module resolver {
compositions?: { target: string }[]
doc?: string,
elements?: { [key: string]: EntityCSN }
key?: string // custom!!
key?: boolean // custom!!
keys?: { [key:string]: any }
kind: string,
includes?: string[]
Expand All @@ -25,6 +25,8 @@ export module resolver {
target?: string,
type: string | ref,
name: string,
'@singular'?: string,
'@plural'?: string,
'@odata.draft.enabled'?: boolean // custom!
_unresolved?: boolean
isRefNotNull?: boolean // custom!
Expand Down
4 changes: 2 additions & 2 deletions lib/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ if (process.version.startsWith('v14')) {

const last = /\w+$/

const annotations = {
const annotations = /** @type {const} */ ({
singular: ['@singular'],
plural: ['@plural'],
}
})

/**
* Converts a camelCase string to snake_case.
Expand Down
Loading
Loading