mirror of
https://github.com/MichMich/MagicMirror.git
synced 2026-06-29 19:43:21 -07:00
fix: modules losing data after HTTP 304 responses (#4180)
When a server responds with 304 (nothing changed since last fetch), the response has no body. Several modules were trying to parse that empty body anyway - which either cleared their cached data or threw an exception. The result: a blank calendar, empty newsfeed, or missing weather data after the next refresh cycle. This was reported in the forum: https://forum.magicmirror.builders/topic/20250/calendar-events-broadcasting-nothing-showing The bug was "introduced" by #4120, which correctly started forwarding 304s to consumers - but not all were ready for it. ### Fix Skip parsing on 304 and keep the existing data as-is: - **calendar** - re-broadcasts cached events - **newsfeed** - re-broadcasts cached items - **buienradar, openmeteo, weatherflow, weathergov** - return early before calling `response.json()`
This commit is contained in:
committed by
GitHub
parent
3fd1591953
commit
5d11b5d73b
@@ -51,6 +51,13 @@ class CalendarFetcher {
|
||||
*/
|
||||
async #handleResponse (response) {
|
||||
try {
|
||||
// 304 Not Modified has no body: keep previously fetched events and just re-broadcast them.
|
||||
if (response.status === 304) {
|
||||
this.lastFetch = Date.now();
|
||||
this.broadcastEvents();
|
||||
return;
|
||||
}
|
||||
|
||||
const responseData = await response.text();
|
||||
const parsed = await ical.async.parseICS(responseData);
|
||||
|
||||
|
||||
@@ -68,6 +68,12 @@ class NewsfeedFetcher {
|
||||
* @param {Response} response - The fetch Response object
|
||||
*/
|
||||
async #handleResponse (response) {
|
||||
// 304 Not Modified has no body: keep previously fetched items and re-broadcast them.
|
||||
if (response.status === 304) {
|
||||
this.broadcastItems();
|
||||
return;
|
||||
}
|
||||
|
||||
this.items = [];
|
||||
const parser = new FeedMe();
|
||||
|
||||
|
||||
@@ -111,6 +111,7 @@ class BuienradarProvider {
|
||||
});
|
||||
|
||||
this.fetcher.on("response", async (response) => {
|
||||
if (response.status === 304) return;
|
||||
try {
|
||||
const data = await response.json();
|
||||
this.#handleResponse(data);
|
||||
|
||||
@@ -69,6 +69,7 @@ class EnvCanadaProvider {
|
||||
});
|
||||
|
||||
this.fetcher.on("response", async (response) => {
|
||||
if (response.status === 304) return;
|
||||
try {
|
||||
// Check if hour changed - restart fetcher with new URL
|
||||
const newHour = new Date().toISOString().substring(11, 13);
|
||||
|
||||
@@ -172,6 +172,7 @@ class OpenMeteoProvider {
|
||||
});
|
||||
|
||||
this.fetcher.on("response", async (response) => {
|
||||
if (response.status === 304) return;
|
||||
try {
|
||||
const data = await response.json();
|
||||
this.#handleResponse(data);
|
||||
|
||||
@@ -73,6 +73,7 @@ class OpenWeatherMapProvider {
|
||||
});
|
||||
|
||||
this.fetcher.on("response", async (response) => {
|
||||
if (response.status === 304) return;
|
||||
try {
|
||||
const data = await response.json();
|
||||
this.#handleResponse(data);
|
||||
|
||||
@@ -52,6 +52,7 @@ class PirateweatherProvider {
|
||||
});
|
||||
|
||||
this.fetcher.on("response", async (response) => {
|
||||
if (response.status === 304) return;
|
||||
try {
|
||||
const data = await response.json();
|
||||
this.#handleResponse(data);
|
||||
|
||||
@@ -95,6 +95,7 @@ class SMHIProvider {
|
||||
});
|
||||
|
||||
this.fetcher.on("response", async (response) => {
|
||||
if (response.status === 304) return;
|
||||
try {
|
||||
const data = await response.json();
|
||||
this.#handleResponse(data);
|
||||
|
||||
@@ -64,6 +64,7 @@ class UkMetOfficeDataHubProvider {
|
||||
});
|
||||
|
||||
this.fetcher.on("response", async (response) => {
|
||||
if (response.status === 304) return;
|
||||
try {
|
||||
const data = await response.json();
|
||||
this.#handleResponse(data);
|
||||
|
||||
@@ -74,6 +74,7 @@ class WeatherFlowProvider {
|
||||
});
|
||||
|
||||
this.fetcher.on("response", async (response) => {
|
||||
if (response.status === 304) return;
|
||||
try {
|
||||
const data = await response.json();
|
||||
const processed = this.#processData(data);
|
||||
|
||||
@@ -207,6 +207,7 @@ class WeatherGovProvider {
|
||||
});
|
||||
|
||||
this.fetcher.on("response", async (response) => {
|
||||
if (response.status === 304) return;
|
||||
try {
|
||||
const data = await response.json();
|
||||
this.#handleResponse(data);
|
||||
|
||||
@@ -0,0 +1,130 @@
|
||||
global.moment = require("moment-timezone");
|
||||
|
||||
const ical = require("node-ical");
|
||||
const moment = require("moment-timezone");
|
||||
const defaults = require("../../../../../js/defaults");
|
||||
|
||||
const CalendarFetcherUtils = require(`../../../../../${defaults.defaultModulesDir}/calendar/calendarfetcherutils`);
|
||||
|
||||
const CalendarFetcher = require(`../../../../../${defaults.defaultModulesDir}/calendar/calendarfetcher`);
|
||||
|
||||
const makeFetcher = (options = {}) => new CalendarFetcher(
|
||||
options.url ?? "http://test.example.com/cal.ics",
|
||||
options.reloadInterval ?? 60000,
|
||||
options.excludedEvents ?? [],
|
||||
options.maximumEntries ?? 10,
|
||||
options.maximumNumberOfDays ?? 365,
|
||||
options.auth ?? null,
|
||||
options.includePastEvents ?? false,
|
||||
options.selfSignedCert ?? false
|
||||
);
|
||||
|
||||
// Triggers a fetch and resolves once the fetcher finishes (success or error).
|
||||
// On error, resolves with the errorInfo object so tests can inspect it.
|
||||
const emitResponse = (fetcher, response) => new Promise((resolve) => {
|
||||
fetcher.onReceive(resolve);
|
||||
fetcher.onError((_, errorInfo) => resolve(errorInfo));
|
||||
fetcher.httpFetcher.emit("response", response);
|
||||
});
|
||||
|
||||
const futureEventICS = () => {
|
||||
const start = moment().add(1, "hour");
|
||||
const end = moment().add(2, "hours");
|
||||
return [
|
||||
"BEGIN:VCALENDAR",
|
||||
"BEGIN:VEVENT",
|
||||
`DTSTART:${start.utc().format("YYYYMMDDTHHmmss")}Z`,
|
||||
`DTEND:${end.utc().format("YYYYMMDDTHHmmss")}Z`,
|
||||
"UID:future-1@test",
|
||||
"SUMMARY:Future Event",
|
||||
"END:VEVENT",
|
||||
"END:VCALENDAR"
|
||||
].join("\r\n");
|
||||
};
|
||||
|
||||
describe("CalendarFetcher", () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
describe("304 handling", () => {
|
||||
it("keeps previously fetched events when a 304 Not Modified response arrives", async () => {
|
||||
const fetcher = makeFetcher();
|
||||
|
||||
await emitResponse(fetcher, new Response(futureEventICS(), { status: 200 }));
|
||||
expect(fetcher.events).toHaveLength(1);
|
||||
|
||||
// 304 Not Modified has an empty body: events must be preserved
|
||||
await emitResponse(fetcher, new Response(null, { status: 304 }));
|
||||
expect(fetcher.events).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe("error handling", () => {
|
||||
it("forwards HTTP fetch errors to onError callback", () => {
|
||||
const fetcher = makeFetcher();
|
||||
const onError = vi.fn();
|
||||
const errorInfo = { errorType: "NETWORK_ERROR", message: "boom" };
|
||||
|
||||
fetcher.onError(onError);
|
||||
fetcher.httpFetcher.emit("error", errorInfo);
|
||||
|
||||
expect(onError).toHaveBeenCalledWith(fetcher, errorInfo);
|
||||
});
|
||||
|
||||
it("keeps existing events and reports PARSE_ERROR when parsing fails", async () => {
|
||||
const fetcher = makeFetcher();
|
||||
|
||||
await emitResponse(fetcher, new Response(futureEventICS(), { status: 200 }));
|
||||
expect(fetcher.events).toHaveLength(1);
|
||||
|
||||
vi.spyOn(ical.async, "parseICS").mockRejectedValueOnce(new Error("invalid ics"));
|
||||
const error = await emitResponse(fetcher, new Response("BROKEN", { status: 200 }));
|
||||
|
||||
expect(fetcher.events).toHaveLength(1);
|
||||
expect(error).toMatchObject({
|
||||
errorType: "PARSE_ERROR",
|
||||
translationKey: "MODULE_ERROR_UNSPECIFIED",
|
||||
url: "http://test.example.com/cal.ics"
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("delegation and refetch", () => {
|
||||
it("delegates fetchCalendar to HTTPFetcher.startPeriodicFetch", () => {
|
||||
const fetcher = makeFetcher();
|
||||
const startSpy = vi.spyOn(fetcher.httpFetcher, "startPeriodicFetch");
|
||||
|
||||
fetcher.fetchCalendar();
|
||||
|
||||
expect(startSpy).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("shouldRefetch respects reload interval boundaries", () => {
|
||||
const fetcher = makeFetcher();
|
||||
|
||||
expect(fetcher.shouldRefetch()).toBe(true);
|
||||
|
||||
fetcher.lastFetch = Date.now() - 59999;
|
||||
expect(fetcher.shouldRefetch()).toBe(false);
|
||||
|
||||
fetcher.lastFetch = Date.now() - 60000;
|
||||
expect(fetcher.shouldRefetch()).toBe(true);
|
||||
});
|
||||
|
||||
it("passes configured filter options to CalendarFetcherUtils.filterEvents", async () => {
|
||||
const excludedEvents = ["Do not show me"];
|
||||
const filterSpy = vi.spyOn(CalendarFetcherUtils, "filterEvents");
|
||||
const fetcher = makeFetcher({ excludedEvents, maximumEntries: 7, maximumNumberOfDays: 30, includePastEvents: true });
|
||||
|
||||
await emitResponse(fetcher, new Response(futureEventICS(), { status: 200 }));
|
||||
|
||||
expect(filterSpy).toHaveBeenCalledWith(expect.any(Object), {
|
||||
excludedEvents,
|
||||
includePastEvents: true,
|
||||
maximumEntries: 7,
|
||||
maximumNumberOfDays: 30
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user