Skip to content

Make newly created session return no bookmarks #844

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

Closed
wants to merge 1 commit into from
Closed

Make newly created session return no bookmarks #844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion packages/core/src/internal/bookmark.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const EMPTY_BOOKMARK = new Bookmark(null)
* @return {string[]} value converted to an array.
*/
function asStringArray(
value?: string | string[] | Array<string> | null
value?: string | (string | undefined)[] | Array<string | undefined> | null
): string[] {
if (!value) {
return []
Expand Down
30 changes: 16 additions & 14 deletions packages/core/src/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ class Session {
private _writeConnectionHolder: ConnectionHolder
private _open: boolean
private _hasTx: boolean
private _lastBookmark: Bookmark
private _bookmarkIn: Bookmark
private _bookmarkOut?: string
Comment on lines +58 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it could be called currentBookmark and lastReceivedBookmark, or something like this. Because the _bookmarkIn is being changed when a new bookmark arrives.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with that 👌 Will do.

private _transactionExecutor: TransactionExecutor
private _impersonatedUser?: string
private _onComplete: (meta: any) => void
Expand Down Expand Up @@ -117,7 +118,7 @@ class Session {
this._open = true
this._hasTx = false
this._impersonatedUser = impersonatedUser
this._lastBookmark = bookmark || Bookmark.empty()
this._bookmarkIn = bookmark || Bookmark.empty()
this._transactionExecutor = _createTransactionExecutor(config)
this._onComplete = this._onCompleteCallback.bind(this)
this._databaseNameResolved = this._database !== ''
Expand Down Expand Up @@ -150,7 +151,7 @@ class Session {
return this._run(validatedQuery, params, connection => {
this._assertSessionIsOpen()
return (connection as Connection).protocol().run(validatedQuery, params, {
bookmark: this._lastBookmark,
bookmark: this._bookmarkIn,
txConfig: autoCommitTxConfig,
mode: this._mode,
database: this._database,
Expand Down Expand Up @@ -271,7 +272,7 @@ class Session {
reactive: this._reactive,
fetchSize: this._fetchSize
})
tx._begin(this._lastBookmark, txConfig)
tx._begin(this._bookmarkIn, txConfig)
return tx
}

Expand All @@ -296,10 +297,10 @@ class Session {
/**
* Return the bookmark received following the last completed {@link Transaction}.
*
* @return {string[]} A reference to a previous transaction.
* @return {string | undefined} A reference to a previous transaction if present.
*/
lastBookmark(): string[] {
return this._lastBookmark.values()
lastBookmark(): string | undefined {
return this._bookmarkOut
}
Comment on lines 297 to 304
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did the interface changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw a recent question from support regarding the Java driver: they wanted to know if they can safely assume that .lastBookmark would only ever return one bookmark. With this signature, there is no doubt about it.


/**
Expand Down Expand Up @@ -360,7 +361,7 @@ class Session {
/**
* Sets the resolved database name in the session context.
* @private
* @param {string|undefined} database The resolved database name
* @param {string | undefined} database The resolved database name
* @returns {void}
*/
_onDatabaseNameResolved(database?: string): void {
Expand All @@ -376,12 +377,13 @@ class Session {
/**
* Update value of the last bookmark.
* @private
* @param {Bookmark} newBookmark - The new bookmark.
* @param {string | undefined} newBookmark - The new bookmark.
* @returns {void}
*/
_updateBookmark(newBookmark?: Bookmark): void {
if (newBookmark && !newBookmark.isEmpty()) {
this._lastBookmark = newBookmark
_updateBookmark(newBookmark?: string): void {
if (newBookmark !== undefined) {
this._bookmarkIn = new Bookmark(newBookmark)
this._bookmarkOut = newBookmark
Comment on lines -382 to +386
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the previous newBookmark will be always defined. So, it could be changed for if(!newBookmark.isEmpty())

Copy link
Member Author

Choose a reason for hiding this comment

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

But what was the !newBookmark.isEmpty() there fore before if not to check if the SUCCESS message that triggered this update actually contains a bookmark?

}
}

Expand Down Expand Up @@ -414,8 +416,8 @@ class Session {
* @param {Object} meta Connection metadatada
* @returns {void}
*/
_onCompleteCallback(meta: { bookmark: string | string[] }): void {
this._updateBookmark(new Bookmark(meta.bookmark))
_onCompleteCallback(meta: { bookmark: string }): void {
this._updateBookmark(meta.bookmark)
}

/**
Expand Down
10 changes: 5 additions & 5 deletions packages/core/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class Transaction {
private _reactive: boolean
private _state: any
private _onClose: () => void
private _onBookmark: (bookmark: Bookmark) => void
private _onBookmark: (bookmark: string | undefined ) => void
private _onConnection: () => void
private _onError: (error: Error) => Promise<Connection | void>
private _onComplete: (metadata: any) => void
Expand All @@ -58,7 +58,7 @@ class Transaction {
* @constructor
* @param {ConnectionHolder} connectionHolder - the connection holder to get connection from.
* @param {function()} onClose - Function to be called when transaction is committed or rolled back.
* @param {function(bookmark: Bookmark)} onBookmark callback invoked when new bookmark is produced.
* @param {function(bookmark: string | undefined)} onBookmark callback invoked when new bookmark is produced.
* * @param {function()} onConnection - Function to be called when a connection is obtained to ensure the conneciton
* is not yet released.
* @param {boolean} reactive whether this transaction generates reactive streams
Expand All @@ -76,7 +76,7 @@ class Transaction {
}: {
connectionHolder: ConnectionHolder
onClose: () => void
onBookmark: (bookmark: Bookmark) => void
onBookmark: (bookmark: string | undefined) => void
onConnection: () => void
reactive: boolean
fetchSize: number
Expand Down Expand Up @@ -226,8 +226,8 @@ class Transaction {
* @param {object} meta The meta with bookmark
* @returns {void}
*/
_onCompleteCallback(meta: { bookmark?: string | string[] }): void {
this._onBookmark(new Bookmark(meta.bookmark))
_onCompleteCallback(meta: { bookmark?: string }): void {
this._onBookmark(meta.bookmark)
Comment on lines -229 to +230
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change onBookmark signature?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I want to fail fast should the server send multiple bookmarks. According to the protocol spec, there should only ever be 0 or 1 bookmark in a SUCCESS message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, thinking further down the call stack, I want _bookmarkOut to be a string. How do you turn a Bookmark (which can contain a string array) into a single string in a type safe way?

}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/neo4j-driver/test/types/session.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const txConfig7: TransactionConfig = {
}

const tx1: Transaction = session.beginTransaction()
const bookmark: string[] = session.lastBookmark()
const bookmark: string | undefined = session.lastBookmark()

const promise1: Promise<number> = session.readTransaction((tx: Transaction) => {
return 10
Expand Down Expand Up @@ -173,4 +173,4 @@ const promise6: Promise<number> = session.writeTransaction(
txConfig4
)

const lastBookmark: string[] = session.lastBookmark()
const lastBookmark: string | undefined = session.lastBookmark()
6 changes: 5 additions & 1 deletion packages/testkit-backend/src/request-handlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,11 @@ export function TransactionRollback (context, data, wire) {
export function SessionLastBookmarks (context, data, wire) {
const { sessionId } = data
const session = context.getSession(sessionId)
const bookmarks = session.lastBookmark()
const bookmark = session.lastBookmark()
const bookmarks = []
if (bookmark !== undefined) {
bookmarks.push(bookmark)
}
wire.writeResponse('Bookmarks', { bookmarks })
}

Expand Down