Skip to content

Fixing duplicate sources bug when creating claim #1795

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

Merged
thesocialdev merged 2 commits into from
Jan 26, 2025
Merged

Fixing duplicate sources bug when creating claim #1795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a9dbb10
Fixing duplicate sources bug when creating claim
LuizFNJ Jan 23, 2025
8212746
refactor code
LuizFNJ Jan 23, 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
10 changes: 4 additions & 6 deletions server/claim/claim-revision/claim-revision.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,17 +204,15 @@ export class ClaimRevisionService {
if (sources && Array.isArray(sources)) {
for (let source of sources) {
try {
if (typeof source === "string") {
const existingSources = await this.sourceService.getSourceByHref(source);
if (existingSources) {
this.sourceService.updateTargetId(existingSources._id, claimId)
Copy link
Collaborator

@pepermao pepermao Jan 28, 2025

Choose a reason for hiding this comment

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

nitpick: consider adding the await keyword in future opportunities

Copy link
Collaborator

@caneppelevitor caneppelevitor Jan 28, 2025

Choose a reason for hiding this comment

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

Awaiting updateTargetId is unnecessary here because its result isn't used, and it's the last operation in this branch, so it doesn't block further processing.

Copy link
Collaborator

@pepermao pepermao Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The try/catch block won't catch possible errors when updating the source target ID if we don't await the response. Following your conclusion, we don't need to await to save documents in the db, since it's the last operation and the result isn't used. However the success or failure status are used in both cases

Copy link
Collaborator

@caneppelevitor caneppelevitor Jan 28, 2025

Choose a reason for hiding this comment

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

Errors will still be caught by the try/catch block, even without await, since promise rejections propagate to the catch.

Copy link
Collaborator

@pepermao pepermao Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Well, not sure about that. could you send me the documentation for that ?

Copy link
Collaborator

@pepermao pepermao Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is a very simple example where the console "save in the database" occurs even though we got an error in the db transaction. The promise execution starts but does not throw an error inside the try block because it's asynchronous. Any errors will be unhandled unless explicitly caught later.

async function test() {
    return new Promise((resolve, reject) => {
        setTimeout(() => {
            console.log('test')
            reject(new Error("error"))
        }, 2000)
    })
}

async function test2() {
    try {
        test()
        console.log("save in the database")
    } catch (e) {
        console.log("ERROR: ", e.message)
    }
}

test2()

// save in the database
// undefined
// VM744:3 test
// VM744:4 Uncaught Error: error
//  at <anonymous>:4:15

Copy link
Collaborator

@pepermao pepermao Jan 28, 2025

Choose a reason for hiding this comment

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

https://javascript.info/async-await

Copy link
Collaborator

@caneppelevitor caneppelevitor Jan 28, 2025

Choose a reason for hiding this comment

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

After looking into this more deeply, I agree that await should be used for proper error handling. Using await ensures that if the underlying function—like updateTargetId—throws or returns a rejected Promise, the error will be caught in the same try/catch block. However, this only works if the function itself actually propagates errors by awaiting its own database calls. If, for instance, we never await source.save() inside updateTargetId, then a DB error in the save step won’t bubble up, even if we do await updateTargetId in the calling code. So, to fully capture errors, both the function and the caller need to consistently use await.

Copy link
Collaborator

@pepermao pepermao Jan 28, 2025

Choose a reason for hiding this comment

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

Nice, could you provide the documentation that you looked at ? I think it's worth to share with everybody else. Thanks for the discussion, it's always good to have these kind of talks, I almost forgot about the try/catch error handling concept

} else {
await this.sourceService.create({
href: source,
targetId: claimId,
targetModel: SourceTargetModel.Claim,
});
} else {
await this.sourceService.updateTargetId(
source._id,
claimId
);
}
} catch (e) {
this.logger.error(e);
Expand Down
4 changes: 4 additions & 0 deletions server/source/source.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ export class SourceService {
return this.SourceModel.find({ match }, { _id: 1, href: 1 });
}

getSourceByHref(href: string) {
return this.SourceModel.findOne({ href: { $eq: href } }).exec();
}

getById(_id) {
return this.SourceModel.findById(_id);
}
Expand Down
Loading